Re: [RFC 0/2] vduse: add support for networking devices

[Jason Wang]

On Thu, Apr 20, 2023 at 10:16 PM Maxime Coquelin
<maxime.coquelin@xxxxxxxxxx> wrote:
>
>
>
> On 4/20/23 06:34, Jason Wang wrote:
> > On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin
> > <maxime.coquelin@xxxxxxxxxx> wrote:
> >>
> >> This small series enables virtio-net device type in VDUSE.
> >> With it, basic operation have been tested, both with
> >> virtio-vdpa and vhost-vdpa using DPDK Vhost library series
> >> adding VDUSE support [0] using split rings layout.
> >>
> >> Control queue support (and so multiqueue) has also been
> >> tested, but require a Kernel series from Jason Wang
> >> relaxing control queue polling [1] to function reliably.
> >>
> >> Other than that, we have identified a few gaps:
> >>
> >> 1. Reconnection:
> >>   a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail
> >>      index, even after the virtqueue has already been
> >>      processed. Is that expected? I have tried instead to
> >>      get the driver's avail index directly from the avail
> >>      ring, but it does not seem reliable as I sometimes get
> >>      "id %u is not a head!\n" warnings. Also such solution
> >>      would not be possible with packed ring, as we need to
> >>      know the wrap counters values.
> >
> > Looking at the codes, it only returns the value that is set via
> > set_vq_state(). I think it is expected to be called before the
> > datapath runs.
> >
> > So when bound to virtio-vdpa, it is expected to return 0. But we need
> > to fix the packed virtqueue case, I wonder if we need to call
> > set_vq_state() explicitly in virtio-vdpa before starting the device.
> >
> > When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which
> > will end up a call to set_vq_state(). Unfortunately, it doesn't
> > support packed ring which needs some extension.
> >
> >>
> >>   b. Missing IOCTLs: it would be handy to have new IOCTLs to
> >>      query Virtio device status,
> >
> > What's the use case of this ioctl? It looks to me userspace is
> > notified on each status change now:
> >
> > static int vduse_dev_set_status(struct vduse_dev *dev, u8 status)
> > {
> >          struct vduse_dev_msg msg = { 0 };
> >
> >          msg.req.type = VDUSE_SET_STATUS;
> >          msg.req.s.status = status;
> >
> >          return vduse_dev_msg_sync(dev, &msg);
> > }
>
> The idea was to be able to query the status at reconnect time, and
> neither having to assume its value nor having to store its value in a
> file (the status could change while the VDUSE application is stopped,
> but maybe it would receive the notification at reconnect).

I see.

>
> I will prototype using a tmpfs file to save needed information, and see
> if it works.

It might work but then the API is not self contained. Maybe it's
better to have a dedicated ioctl.

>
> >> and retrieve the config
> >>      space set at VDUSE_CREATE_DEV time.
> >
> > In order to be safe, VDUSE avoids writable config space. Otherwise
> > drivers could block on config writing forever. That's why we don't do
> > it now.
>
> The idea was not to make the config space writable, but just to be able
> to fetch what was filled at VDUSE_CREATE_DEV time.
>
> With the tmpfs file, we can avoid doing that and just save the config
> space there.

Same as the case for status.

Thanks

>
> > We need to harden the config write before we can proceed to this I think.
> >
> >>
> >> 2. VDUSE application as non-root:
> >>    We need to run the VDUSE application as non-root. There
> >>    is some race between the time the UDEV rule is applied
> >>    and the time the device starts being used. Discussing
> >>    with Jason, he suggested we may have a VDUSE daemon run
> >>    as root that would create the VDUSE device, manages its
> >>    rights and then pass its file descriptor to the VDUSE
> >>    app. However, with current IOCTLs, it means the VDUSE
> >>    daemon would need to know several information that
> >>    belongs to the VDUSE app implementing the device such
> >>    as supported Virtio features, config space, etc...
> >>    If we go that route, maybe we should have a control
> >>    IOCTL to create the device which would just pass the
> >>    device type. Then another device IOCTL to perform the
> >>    initialization. Would that make sense?
> >
> > I think so. We can hear from others.
> >
> >>
> >> 3. Coredump:
> >>    In order to be able to perform post-mortem analysis, DPDK
> >>    Vhost library marks pages used for vrings and descriptors
> >>    buffers as MADV_DODUMP using madvise(). However with
> >>    VDUSE it fails with -EINVAL. My understanding is that we
> >>    set VM_DONTEXPAND flag to the VMAs and madvise's
> >>    MADV_DODUMP fails if it is present. I'm not sure to
> >>    understand why madvise would prevent MADV_DODUMP if
> >>    VM_DONTEXPAND is set. Any thoughts?
> >
> > Adding Peter who may know the answer.
>
> Thanks!
> Maxime
>
> > Thanks
> >
> >>
> >> [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both
> >> [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@xxxxxxxxxxxxxx/T/
> >>
> >> Maxime Coquelin (2):
> >>    vduse: validate block features only with block devices
> >>    vduse: enable Virtio-net device type
> >>
> >>   drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++----
> >>   1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> --
> >> 2.39.2
> >>
> >
>