Re: [RFC PATCH 05/27] vhost: Add hdev->dev.sw_lm_vq_handler

[Eugenio Perez Martin <eperezma@redhat.com>]

On Mon, Dec 7, 2020 at 5:52 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 07:50:43PM +0100, Eugenio Pérez wrote:
> > Only virtio-net honors it.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  include/hw/virtio/vhost.h |  1 +
> >  hw/net/virtio-net.c       | 39 ++++++++++++++++++++++++++++-----------
> >  2 files changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 4a8bc75415..b5b7496537 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -83,6 +83,7 @@ struct vhost_dev {
> >      bool started;
> >      bool log_enabled;
> >      uint64_t log_size;
> > +    VirtIOHandleOutput sw_lm_vq_handler;
>
> sw == software?
> lm == live migration?
>
> Maybe there is a name that is clearer. What are these virtqueues called?
> Shadow vqs? Logged vqs?
>
> Live migration is a feature that uses dirty memory logging, but other
> features may use dirty memory logging too. The name should probably not
> be associated with live migration.
>

I totally agree, I find shadow_vq a better name for it.

> >      Error *migration_blocker;
> >      const VhostOps *vhost_ops;
> >      void *opaque;
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 9179013ac4..9a69ae3598 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -2628,24 +2628,32 @@ static void virtio_net_tx_bh(void *opaque)
> >      }
> >  }
> >
> > -static void virtio_net_add_queue(VirtIONet *n, int index)
> > +static void virtio_net_add_queue(VirtIONet *n, int index,
> > +                                 VirtIOHandleOutput custom_handler)
> >  {
>
> We talked about the possibility of moving this into the generic vhost
> code so that devices don't need to be modified. It would be nice to hide
> this feature inside vhost.

I'm thinking of tying it to VirtQueue, allowing the caller to override
the handler knowing it is not going to be called (I mean, not offering
race conditions protection, like before of starting processing
notifications in qemu calling vhost_dev_disable_notifiers).

Thanks!