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

[Stefan Hajnoczi <stefanha@gmail.com>]

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.

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.

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.

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