Re: [PATCH v4 1/5] extend libvhost to support IOThread and coroutine

[Stefan Hajnoczi]

On Tue, Feb 18, 2020 at 01:07:07PM +0800, Coiby Xu wrote:
> +static bool vu_message_read(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
> +{
> +    vu_read_msg_cb read_msg;
> +    if (dev->co_iface) {
> +        read_msg = dev->co_iface->read_msg;
> +    } else {
> +        read_msg = vu_message_read_;
> +    }
> +    return read_msg(dev, conn_fd, vmsg);
> +}

libvhost-user is already partially asynchronous (->set/remove_watch()
are used for eventfds).

Can you make the vhost-user socket I/O asynchronous too?

> +
>  static bool
>  vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>  {
> @@ -1075,9 +1084,14 @@ vu_set_vring_kick_exec(VuDev *dev, VhostUserMsg *vmsg)
>      }
>  
>      if (dev->vq[index].kick_fd != -1 && dev->vq[index].handler) {
> +        if (dev->set_watch_packed_data) {
> +            dev->set_watch_packed_data(dev, dev->vq[index].kick_fd, 
> VU_WATCH_IN,
> +                                       dev->co_iface->kick_callback,
> +                                       (void *)(long)index);
> +        } else {
>          dev->set_watch(dev, dev->vq[index].kick_fd, VU_WATCH_IN,
>                         vu_kick_cb, (void *)(long)index);

Why is it necessary to replace vu_kick_cb()?

The user can set a custom vq->handler() function with
vu_set_queue_handler().

Coroutines aren't needed for this part since eventfd_read() always
returns right away.

> @@ -1627,6 +1647,12 @@ vu_deinit(VuDev *dev)
>          }
>  
>          if (vq->kick_fd != -1) {
> +            /* remove watch for kick_fd
> +             * When client process is running in gdb and
> +             * quit command is run in gdb, QEMU will still dispatch the event
> +             * which will cause segment fault in the callback function
> +             */

Code and comments in libvhost-user should not refer to QEMU specifics.
Removing the watch is a good idea regardless of the application or event
loop implementation.  No comment is needed here.

> +            dev->remove_watch(dev, vq->kick_fd);
>              close(vq->kick_fd);
>              vq->kick_fd = -1;
>          }
> @@ -1682,7 +1708,7 @@ vu_init(VuDev *dev,
>  
>      assert(max_queues > 0);
>      assert(socket >= 0);
> -    assert(set_watch);
> +    /* assert(set_watch); */

?

> @@ -372,7 +389,8 @@ struct VuDev {
>      /* @set_watch: add or update the given fd to the watch set,
>       * call cb when condition is met */
>      vu_set_watch_cb set_watch;
> -
> +    /* AIO dispatch will only one data pointer to callback function */

I don't understand what this comment is saying.

signature.asc
Description: PGP signature