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

[Jason Wang <jasowang@redhat.com>]

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;

Thanks
>
> Thanks,
> Aurelien
>