Re: [PATCH 04/31] vdpa: Add vhost_svq_set_svq_kick_fd

[Eugenio Perez Martin]

On Fri, Jan 28, 2022 at 7:29 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> > This function allows the vhost-vdpa backend to override kick_fd.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.h |  1 +
> >   hw/virtio/vhost-shadow-virtqueue.c | 45 ++++++++++++++++++++++++++++++
> >   2 files changed, 46 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> > b/hw/virtio/vhost-shadow-virtqueue.h
> > index 400effd9f2..a56ecfc09d 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -15,6 +15,7 @@
> >
> >   typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> >
> > +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
> >   const EventNotifier *vhost_svq_get_dev_kick_notifier(
> >                                                 const VhostShadowVirtqueue 
> > *svq);
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index bd87110073..21534bc94d 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -11,6 +11,7 @@
> >   #include "hw/virtio/vhost-shadow-virtqueue.h"
> >
> >   #include "qemu/error-report.h"
> > +#include "qemu/main-loop.h"
> >
> >   /* Shadow virtqueue to relay notifications */
> >   typedef struct VhostShadowVirtqueue {
> > @@ -18,8 +19,20 @@ typedef struct VhostShadowVirtqueue {
> >       EventNotifier hdev_kick;
> >       /* Shadow call notifier, sent to vhost */
> >       EventNotifier hdev_call;
> > +
> > +    /*
> > +     * Borrowed virtqueue's guest to host notifier.
> > +     * To borrow it in this event notifier allows to register on the event
> > +     * loop and access the associated shadow virtqueue easily. If we use 
> > the
> > +     * VirtQueue, we don't have an easy way to retrieve it.
> > +     *
> > +     * So shadow virtqueue must not clean it, or we would lose VirtQueue 
> > one.
> > +     */
> > +    EventNotifier svq_kick;
> >   } VhostShadowVirtqueue;
> >
> > +#define INVALID_SVQ_KICK_FD -1
> > +
> >   /**
> >    * The notifier that SVQ will use to notify the device.
> >    */
> > @@ -29,6 +42,35 @@ const EventNotifier *vhost_svq_get_dev_kick_notifier(
> >       return &svq->hdev_kick;
> >   }
> >
> > +/**
> > + * Set a new file descriptor for the guest to kick SVQ and notify for avail
> > + *
> > + * @svq          The svq
> > + * @svq_kick_fd  The new svq kick fd
> > + */
> > +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd)
> > +{
> > +    EventNotifier tmp;
> > +    bool check_old = INVALID_SVQ_KICK_FD !=
> > +                     event_notifier_get_fd(&svq->svq_kick);
> > +
> > +    if (check_old) {
> > +        event_notifier_set_handler(&svq->svq_kick, NULL);
> > +        event_notifier_init_fd(&tmp, 
> > event_notifier_get_fd(&svq->svq_kick));
> > +    }
>
>
> It looks to me we don't do similar things in vhost-net. Any reason for
> caring about the old svq_kick?
>

Do you mean to check for old kick_fd in case we miss notifications,
and explicitly omit the INVALID_SVQ_KICK_FD?

If you mean qemu's vhost-net, I guess it's because the device's kick
fd is never changed in all the vhost device lifecycle, it's only set
at the beginning. Previous RFC also depended on that, but you
suggested better vhost and SVQ in v4 feedback if I understood
correctly [1]. Or am I missing something?

Qemu's vhost-net does not need to use this because it is not polling
it. For kernel's vhost, I guess the closest is the use of pollstop and
pollstart at vhost_vring_ioctl.

In my opinion, I think that SVQ code size can benefit from now
allowing to override kick_fd from the start of the operation. Not from
initialization, but start. But I can see the benefits of having the
change into account from this moment so it's more resilient to the
future.

>
> > +
> > +    /*
> > +     * event_notifier_set_handler already checks for guest's notifications 
> > if
> > +     * they arrive to the new file descriptor in the switch, so there is no
> > +     * need to explicitely check for them.
> > +     */
> > +    event_notifier_init_fd(&svq->svq_kick, svq_kick_fd);
> > +
> > +    if (!check_old || event_notifier_test_and_clear(&tmp)) {
> > +        event_notifier_set(&svq->hdev_kick);
>
>
> Any reason we need to kick the device directly here?
>

At this point of the series only notifications are forwarded, not
buffers. If kick_fd is set, we need to check the old one, the same way
as vhost checks the masked notifier in case of change.

Thanks!

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg03152.html
, from "I'd suggest to not depend on this since it:"


> Thanks
>
>
> > +    }
> > +}
> > +
> >   /**
> >    * Creates vhost shadow virtqueue, and instruct vhost device to use the 
> > shadow
> >    * methods and file descriptors.
> > @@ -52,6 +94,9 @@ VhostShadowVirtqueue *vhost_svq_new(void)
> >           goto err_init_hdev_call;
> >       }
> >
> > +    /* Placeholder descriptor, it should be deleted at set_kick_fd */
> > +    event_notifier_init_fd(&svq->svq_kick, INVALID_SVQ_KICK_FD);
> > +
> >       return g_steal_pointer(&svq);
> >
> >   err_init_hdev_call:
>