On 10/3/23 16:25, Michael S. Tsirkin wrote: > On Tue, Oct 03, 2023 at 03:23:24PM +0200, Laszlo Ersek wrote: >> On 10/3/23 15:08, 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. >>> >>> 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.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. >>> >>> 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? >> >> ... from a vhost-user backend perspective, won't this create a need for >> all "ring processor" (~ virtio event loop) implementations to support >> both methods? IIUC, the "virtio pop" is usually independent of the >> particular device to which the requests are ultimately delivered. So the >> event loop would have to grow a new parameter regarding "what to do in >> the started-but-disabled state", the network device would have to pass >> in one value (-> pop & drop), and all other devices would have to pass >> in the other value (stop popping). >> >> ... I figure in rust-vmm/vhost it would affect the "handle_event" >> function in "crates/vhost-user-backend/src/event_loop.rs". >> >> Do I understand right? (Not disagreeing, just pondering the impact on >> backends.) >> >> Laszlo > > Already the case I guess - RX ring is not processed, TX is. Right? >
Ah I see your point, this distinction must already exist in event loops. But... as far as I can tell, it's not there in rust-vmm/vhost. Laszlo