Re: [Qemu-devel] [PATCH V2 18/20] virtio-net: multiqueue support

[Jason Wang <jasowang@redhat.com>]

On 04/17/2013 04:42 PM, Aurelien Jarno wrote:
> On Wed, Apr 17, 2013 at 04:27:13PM +0800, Jason Wang wrote:
>> On 04/17/2013 03:20 PM, Aurelien Jarno wrote:
>>> On Tue, Apr 16, 2013 at 11:08:59AM +0800, Jason Wang wrote:
>>>> On 04/15/2013 04:28 PM, Aurelien Jarno wrote:
>>>>> On Mon, Apr 15, 2013 at 01:29:01PM +0800, Jason Wang wrote:
>>>>>> On 04/13/2013 09:17 PM, Aurelien Jarno wrote:
>>>>>>> On Fri, Jan 25, 2013 at 06:35:41PM +0800, Jason Wang wrote:
>>>>>>>> This patch implements both userspace and vhost support for multiple queue
>>>>>>>> virtio-net (VIRTIO_NET_F_MQ). This is done by introducing an array of
>>>>>>>> VirtIONetQueue to VirtIONet.
>>>>>>>>
>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>> ---
>>>>>>>>  hw/virtio-net.c |  317 +++++++++++++++++++++++++++++++++++++++++++------------
>>>>>>>>  hw/virtio-net.h |   28 +++++-
>>>>>>>>  2 files changed, 275 insertions(+), 70 deletions(-)
>>>>>>> This patch breaks virtio-net in Minix, even with multiqueue disable. I
>>>>>>> don't know virtio enough to know if it is a Minix or a QEMU problem.
>>>>>>> However I have been able to identify the part of the commit causing the
>>>>>>> failure:
>>>>>> Hi Aurelien:
>>>>>>
>>>>>> Thanks for the work.
>>>>>>>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>>>>>>>> index ef522d5..cec91a7 100644
>>>>>>>> --- a/hw/virtio-net.c
>>>>>>>> +++ b/hw/virtio-net.c
>>>>>>> ...
>>>>>>>
>>>>>>>> +static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl)
>>>>>>>> +{
>>>>>>>> +    VirtIODevice *vdev = &n->vdev;
>>>>>>>> +    int i, max = multiqueue ? n->max_queues : 1;
>>>>>>>> +
>>>>>>>> +    n->multiqueue = multiqueue;
>>>>>>>> +
>>>>>>>> +    for (i = 2; i <= n->max_queues * 2 + 1; i++) {
>>>>>>>> +        virtio_del_queue(vdev, i);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>> The for loop above is something which is new, even with multiqueue
>>>>>>> disabled. Even with max_queues=1 it calls virtio_del_queue with i = 2
>>>>>>> and i = 3. Disabling this loop makes the code to work as before.
>>>>>> Looks like a bug here, need to change n->max_queues * 2 + 1 to
>>>>>> n->max_queues * 2. The reason we need to del queue 2 each time because
>>>>>> vq 2 has different meaning is multiqueue and single queue. In single
>>>>>> queue, vq 2 maybe ctrl vq, but in multiqueue mode it was rx1.
>>>>>>
>>>>>> Let's see whether this small change works.
>>>>> Unfortunately it doesn't fix the issue. I don't know a lot about virtio,
>>>>> but would it be possible to only delete the queue that have been
>>>>> enabled?
>>>> The issue is vq 2 has different meanings in two modes, so it must be
>>>> deleted and reinitialized during feature negotiation.
>>>>>>> On the Minix side it triggers the following assertion:
>>>>>>>
>>>>>>> | virtio.c:370: assert "q->vaddr != NULL" failed, function "free_phys_queue"                                                                                                                                 
>>>>>>> | virtio_net(73141): panic: assert failed
>>>>>>>
>>>>>>> This correspond to this function in lib/libvirtio/virtio.c:
>>>>>>>
>>>>>>> | static void
>>>>>>> | free_phys_queue(struct virtio_queue *q)
>>>>>>> | {
>>>>>>> |         assert(q != NULL);
>>>>>>> |         assert(q->vaddr != NULL);
>>>>>>> | 
>>>>>>> |         free_contig(q->vaddr, q->ring_size);
>>>>>>> |         q->vaddr = NULL;
>>>>>>> |         q->paddr = 0;
>>>>>>> |         q->num = 0;
>>>>>>> |         free_contig(q->data, sizeof(q->data[0]));
>>>>>>> |         q->data = NULL;
>>>>>>> | }
>>>>>>>
>>>>>>> Do you have an idea if the problem is on the Minix side or on the QEMU
>>>>>>> side?
>>>>>>>
>>>>>> Haven't figured out the relationship between virtqueue dynamic del/add
>>>>>> and q->vaddr here. If the above changes does not work, I guess this
>>>>> Unfortunately I don't really know the minix code either. I happen to
>>>>> found the issue when testing other changes, and looked at the code to
>>>>> try to understand the problem.
>>>> Me too.
>>>>>> problem happens only for ctrl vq (vq2)? And when does this happen?
>>>>> I guess so given removing the call to virtio_del_queue() for vq2 fixes
>>>>> or workarounds the issue.
>>>> It will cause trouble for multiqueue guest who may think vq2 is rx queue.
>>>>>> Rebooting?
>>>>> It happens at the initial boot.
>>>> Looks at the codes, looks like vq2 was initialized unconditionally at
>>>> start, how about the following patch?
>>>>
>>>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>>>> index 4bb49eb..4886aa0 100644
>>>> --- a/hw/virtio-net.c
>>>> +++ b/hw/virtio-net.c
>>>> @@ -1300,7 +1300,6 @@ VirtIODevice *virtio_net_init(DeviceState *dev,
>>>> NICConf *conf,
>>>>                                             virtio_net_handle_tx_bh);
>>>>          n->vqs[0].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[0]);
>>>>      }
>>>> -    n->ctrl_vq = virtio_add_queue(&n->vdev, 64, virtio_net_handle_ctrl);
>>>>      qemu_macaddr_default_if_unset(&conf->macaddr);
>>>>      memcpy(&n->mac[0], &conf->macaddr, sizeof(n->mac));
>>>>      n->status = VIRTIO_NET_S_LINK_UP;
>>>>
>>> Unfortunately this patch doesn't work, I tried it with and without your
>>> previous suggested change (max_queues * 2 + 1) => max_queues * 2.
>>>
>>> Aurelien
>>>
>> Thanks for the test, I will try to get a minix to see what happens and
>> keep you updated.
>>
> Thanks! I have used minix 3.2.1 which include virtio drivers. See the
> following page, step 5 to know how to enable virtio:
>
> http://wiki.minix3.org/en/UsersGuide/RunningOnQemu
>

Not sure whether it's a bug of minix guest, since it always tries to
identify the control vq even if it does not support it:

>From virtio_net_probe():

    /* If the host supports the control queue, allocate it as well */
    if (virtio_host_supports(net_dev, VIRTIO_NET_F_CTRL_VQ))
        queues += 1;

This violates the virtio spec:
" If the VIRTIO_NET_F_CTRL_VQ feature bit is negotiated, identify the
control virtqueue."

The multiqueue patchset conditionally allocate control vq depends on
whether guest support it instead of always allocating it. I will send a
patch to keep this behaviour to preserve backward compatibility.

Thanks