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?
--
MST
