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

[Laszlo Ersek <lersek@redhat.com>]

On 10/3/23 15:08, 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.
> 
> 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?

... 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