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

[Laszlo Ersek]

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