On Tue, Oct 03, 2023 at 09:08:15AM -0400, Stefan Hajnoczi wrote: > On Tue, 3 Oct 2023 at 08:27, Michael S. Tsirkin <m...@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.ouy...@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 <m...@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