Re: [PATCH 7/7] vhost-user: call VHOST_USER_SET_VRING_ENABLE synchronously

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 03, 2023 at 09:08:15AM -0400, Stefan Hajnoczi wrote:
> On Tue, 3 Oct 2023 at 08:27, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Oct 02, 2023 at 05:13:26PM -0400, Stefan Hajnoczi wrote:
> > > One more question:
> > >
> > > Why is the disabled state not needed by regular (non-vhost) virtio-net devices?
> >
> > Tap does the same - it purges queued packets:
> >
> > int tap_disable(NetClientState *nc)
> > {
> >     TAPState *s = DO_UPCAST(TAPState, nc, nc);
> >     int ret;
> >
> >     if (s->enabled == 0) {
> >         return 0;
> >     } else {
> >         ret = tap_fd_disable(s->fd);
> >         if (ret == 0) {
> >             qemu_purge_queued_packets(nc);
> >             s->enabled = false;
> >             tap_update_fd_handler(s);
> >         }
> >         return ret;
> >     }
> > }
> 
> tap_disable() is not equivalent to the vhost-user "started but
> disabled" ring state. tap_disable() is a synchronous one-time action,
> while "started but disabled" is a continuous state.

well, yes. but practically guests do not queue too many buffers
after disabling a queue. I don't know if they reliably don't
or it's racy and we didn't notice it yet - I think it
was mostly dpdk that had this and that's usually
used with vhost-user.

> The "started but disabled" ring state isn't needed to achieve this.
> The back-end can just drop tx buffers upon receiving
> VHOST_USER_SET_VRING_ENABLE .num=0.

yes, maybe that would have been a better way to do this.


> The history of the spec is curious. VHOST_USER_SET_VRING_ENABLE was
> introduced before the the "started but disabled" state was defined,
> and it explicitly mentions tap attach/detach:
> 
> commit 7263a0ad7899994b719ebed736a1119cc2e08110
> Author: Changchun Ouyang <changchun.ouyang@intel.com>
> Date:   Wed Sep 23 12:20:01 2015 +0800
> 
>     vhost-user: add a new message to disable/enable a specific virt queue.
> 
>     Add a new message, VHOST_USER_SET_VRING_ENABLE, to enable or disable
>     a specific virt queue, which is similar to attach/detach queue for
>     tap device.
> 
> and then later:
> 
> commit c61f09ed855b5009f816242ce281fd01586d4646
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Mon Nov 23 12:48:52 2015 +0200
> 
>     vhost-user: clarify start and enable
> 
> >
> > what about non tap backends? I suspect they just aren't
> > used widely with multiqueue so no one noticed.
> 
> I still don't understand why "started but disabled" is needed instead
> of just two ring states: enabled and disabled.

With dropping packets when ring is disabled? Maybe that would
have been enough. I also failed to realize it's specific to
net, seemed generic to me :(

> It seems like the cleanest path going forward is to keep the "ignore
> rx, discard tx" semantics for virtio-net devices but to clarify in the
> spec that other device types do not process the ring:
> 
> "
> * started but disabled: the back-end must not process the ring. For legacy
>   reasons there is an exception for the networking device, where the
>   back-end must process and discard any TX packets and not process
>   other rings.
> "
> 
> What do you think?
> 
> Stefan

Okay... I hope we are not missing any devices which need virtio net
semantics. Care checking them all?

-- 
MST