qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Aurelien Jarno
Subject: Re: [Qemu-devel] [PATCH V2 18/20] virtio-net: multiqueue support
Date: Wed, 17 Apr 2013 09:20:11 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]