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

[Eugenio Perez Martin]

On Tue, Feb 8, 2022 at 9:48 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/1/31 下午6:18, 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?
>
>
> Yes.
>
>
> >
> > 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?
>
>
> No, I forgot that. But in this case we should have a better dealing with
> the the conversion from valid fd to -1 by disabling the handler.
>

Sure, I will do it that way for the next version.

>
> >
> > 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.
>
>
> I meant we need to kick the svq instead of vhost-vdpa in this case?
>

Actually, yes, you're right.

At this moment of the series is not needed, since SVQ will only relay
the kick to the device. But when SVQ starts to forward buffers that's
needed, so thanks for the catch!

> Thanks
>
>
> >
> > 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:
>