Re: [Qemu-devel] [PATCH] virtio-net: do not start queues that are not enabled by the guest

[Jason Wang <jasowang@redhat.com>]

On 2019/2/21 下午2:00, Yuri Benditovich wrote:
> On Tue, Feb 19, 2019 at 8:27 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2019/2/19 上午7:34, Michael S. Tsirkin wrote:
>>> On Mon, Feb 18, 2019 at 10:49:08PM +0200, Yuri Benditovich wrote:
>>>> On Mon, Feb 18, 2019 at 6:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Mon, Feb 18, 2019 at 11:58:51AM +0200, Yuri Benditovich wrote:
>>>>>> On Mon, Feb 18, 2019 at 5:49 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>> On 2019/2/13 下午10:51, Yuri Benditovich wrote:
>>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1608226
>>>>>>>> On startup/link-up in multiqueue configuration the virtio-net
>>>>>>>> tries to starts all the queues, including those that the guest
>>>>>>>> will not enable by VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET.
>>>>>>>> If the guest driver does not allocate queues that it will not
>>>>>>>> use (for example, Windows driver does not) and number of actually
>>>>>>>> used queues is less that maximal number supported by the device,
>>>>>>> Is this a requirement of e.g NDIS? If not, could we simply allocate all
>>>>>>> queues in this case. This is usually what normal Linux driver did.
>>>>>>>
>>>>>>>
>>>>>>>> this causes vhost_net_start to fail and actually disables vhost
>>>>>>>> for all the queues, reducing the performance.
>>>>>>>> Current commit fixes this: initially only first queue is started,
>>>>>>>> upon VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET started all the queues
>>>>>>>> requested by the guest.
>>>>>>>>
>>>>>>>> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>>>>>>>> ---
>>>>>>>>     hw/net/virtio-net.c | 7 +++++--
>>>>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>>>> index 3f319ef723..d3b1ac6d3a 100644
>>>>>>>> --- a/hw/net/virtio-net.c
>>>>>>>> +++ b/hw/net/virtio-net.c
>>>>>>>> @@ -174,7 +174,7 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>>>>>>     {
>>>>>>>>         VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>>>>>>         NetClientState *nc = qemu_get_queue(n->nic);
>>>>>>>> -    int queues = n->multiqueue ? n->max_queues : 1;
>>>>>>>> +    int queues = n->multiqueue ? n->curr_queues : 1;
>>>>>>>>
>>>>>>>>         if (!get_vhost_net(nc->peer)) {
>>>>>>>>             return;
>>>>>>>> @@ -1016,9 +1016,12 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
>>>>>>>>             return VIRTIO_NET_ERR;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -    n->curr_queues = queues;
>>>>>>>>         /* stop the backend before changing the number of queues to avoid handling a
>>>>>>>>          * disabled queue */
>>>>>>>> +    virtio_net_set_status(vdev, 0);
>>>>>>> Any reason for doing this?
>>>>>> I think there are 2 reasons:
>>>>>> 1. The spec does not require guest SW to allocate unused queues.
>>>>>> 2. We spend guest's physical memory to just make vhost happy when it
>>>>>> touches queues that it should not use.
>>>>>>
>>>>>> Thanks,
>>>>>> Yuri Benditovich
>>>>> The spec also says:
>>>>>           queue_enable The driver uses this to selectively prevent the device from executing requests from this
>>>>>           virtqueue. 1 - enabled; 0 - disabled.
>>>>>
>>>>> While this is not a conformance clause this strongly implies that
>>>>> queues which are not enabled are never accessed by device.
>>>>>
>>>>> Yuri I am guessing you are not enabling these unused queues right?
>>>> Of course, we (Windows driver) do not.
>>>> The code of virtio-net passes max_queues to vhost and this causes
>>>> vhost to try accessing all the queues, fail on unused ones and finally
>>>> leave vhost disabled at all.
>>> Jason, at least for 1.0 accessing disabled queues looks like a spec
>>> violation. What do you think?
>>
>> Yes, but there's some issues:
>>
>> - How to detect a disabled queue for 0.9x device? Looks like there's no
>> way according to the spec, so device must assume all queues was enabled.
> Can you please add several words - what is 0.9 device (probably this
> is more about driver) and
> what is the problem with it?


It's not a net specific issue. 0.9x device is legacy device defined in 
the spec. We don't have a method to disable and enable a specific queue 
at that time. Michael said we can assume queue address 0 as disabled, 
but there's still a question of how to enable it. Spec is unclear and it 
was too late to add thing for legacy device. For 1.0 device we have 
queue_enable, but its implementation is incomplete, since it can work 
with vhost correctly, we probably need to add thing to make it work.


>
>> - For 1.0, if we depends on queue_enable, we should implement the
>> callback for vhost I think. Otherwise it's still buggy.
>>
>> So it looks tricky to enable and disable queues through set status
> If I succeed to modify the patch such a way that it will act only in
> 'target' case,
> i.e. only if some of queueus are not initialized (at time of
> driver_ok), will it be more safe?


For 1.0 device, we can fix the queue_enable, but for 0.9x device how do 
you enable one specific queue in this case? (setting status?)

A fundamental question is what prevents you from just initialization all 
queues during driver start? It looks to me this save lots of efforts 
than allocating queue dynamically.

Thanks


>
>> Thanks
>>
>>
>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +    n->curr_queues = queues;
>>>>>>>> +
>>>>>>>>         virtio_net_set_status(vdev, vdev->status);
>>>>>>>>         virtio_net_set_queues(n);
>>>>>>>>