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

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

[Aurelien Jarno]

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:

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

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?

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

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

[Jason Wang]

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.
>
> 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
problem happens only for ctrl vq (vq2)? And when does this happen?
Rebooting?

Thanks a lot

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

[Aurelien Jarno]

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?

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

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

> Rebooting?

It happens at the initial boot.

Thanks,
Aurelien

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

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

[Jason Wang]

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
>

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

[Aurelien Jarno]

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

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

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

[Jason Wang]

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.

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

[Aurelien Jarno]

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

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

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

[Jason Wang]

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