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 <address@hidden>
> >> ---
> >>  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
address@hidden                 http://www.aurel32.net