From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54989) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFHnm-0000nm-Uw for qemu-devel@nongnu.org; Wed, 15 Jul 2015 04:12:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFHnj-0002yl-Kc for qemu-devel@nongnu.org; Wed, 15 Jul 2015 04:12:26 -0400 Received: from [59.151.112.132] (port=61991 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFHni-0002yK-MN for qemu-devel@nongnu.org; Wed, 15 Jul 2015 04:12:23 -0400 Message-ID: <55A61179.5050609@cn.fujitsu.com> Date: Wed, 15 Jul 2015 15:53:29 +0800 From: Wen Congyang MIME-Version: 1.0 References: <55A5FC6E.1030405@cn.fujitsu.com> <55A604DD.4080304@redhat.com> <55A60810.30306@cn.fujitsu.com> <55A60EAA.10205@redhat.com> In-Reply-To: <55A60EAA.10205@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-net: remove virtio queues if the guest doesn't support multiqueue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu-devl , "Michael S. Tsirkin" Cc: Fam Zheng On 07/15/2015 03:41 PM, Jason Wang wrote: > > > On 07/15/2015 03:13 PM, Wen Congyang wrote: >> On 07/15/2015 02:59 PM, Jason Wang wrote: >>> >>> On 07/15/2015 02:23 PM, Wen Congyang wrote: >>>> commit da51a335 adds all queues in .realize(). But if the >>>> guest doesn't support multiqueue, we forget to remove them. And >>>> we cannot handle the ctrl vq corretly. The guest will hang. >>>> >>>> Signed-off-by: Wen Congyang >>> Thanks for the patch, some questions, see below. >>> >>>> --- >>>> hw/net/virtio-net.c | 69 +++++++++++++++++++++++++++++++++++++++++------------ >>>> 1 file changed, 54 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>> index e3c2db3..658806a 100644 >>>> --- a/hw/net/virtio-net.c >>>> +++ b/hw/net/virtio-net.c >>>> @@ -1306,9 +1306,62 @@ static void virtio_net_tx_bh(void *opaque) >>>> } >>>> } >>>> >>>> +static void virtio_net_add_queue(VirtIONet *n, int index) >>>> +{ >>>> + VirtIODevice *vdev = VIRTIO_DEVICE(n); >>>> + >>>> + n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx); >>>> + if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) { >>>> + n->vqs[index].tx_vq = >>>> + virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer); >>>> + n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, >>>> + virtio_net_tx_timer, >>>> + &n->vqs[index]); >>>> + } else { >>>> + n->vqs[index].tx_vq = >>>> + virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh); >>>> + n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]); >>>> + } >>>> + >>>> + n->vqs[index].tx_waiting = 0; >>>> + n->vqs[index].n = n; >>>> +} >>>> + >>>> +static void virtio_net_change_num_queues(VirtIONet *n, int new_max_queues) >>>> +{ >>>> + VirtIODevice *vdev = VIRTIO_DEVICE(n); >>>> + int old_num_queues = virtio_get_num_queues(vdev); >>>> + int new_num_queues = new_max_queues * 2 + 1; >>>> + int i; >>>> + >>>> + assert(old_num_queues >= 3); >>>> + assert(old_num_queues % 2 == 1); >>>> + >>>> + if (old_num_queues == new_num_queues) { >>>> + return; >>>> + } >>>> + >>>> + /* Remove ctrl_vq first */ >>>> + virtio_del_queue(vdev, old_num_queues - 1); >>> Why ctrl vq must be first to be deleted? >> It just make the codes clean. We always need to remove and add ctrl vq if >> old_num_queues != new_num_queues. We only enter one of the two loops. >> >>>> + >>>> + for (i = new_num_queues - 1; i < old_num_queues - 1; i++) { >>>> + virtio_del_queue(vdev, i); >>> Do we need to delete bh and timer also here? >> I think we should delete it too. Before commit da51a335, we only call >> virtio_del_queue(), so I forgot to do it. >> >> Thanks >> Wen Congyang > > I believe we need this patch for 2.4. So could you please do this in V2? OK, I am updating it now. Just wait for a moment. Thanks Wen Congyang > > Thanks >> >>>> + } >>>> + >>>> + for (i = old_num_queues - 1; i < new_num_queues - 1; i += 2) { >>>> + virtio_net_add_queue(n, i / 2); >>>> + } >>>> + >>>> + /* add ctrl_vq last */ >>>> + n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); >>>> +} >>>> + >>>> static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue) >>>> { >>>> + int max = multiqueue ? n->max_queues : 1; >>>> + >>>> n->multiqueue = multiqueue; >>>> + virtio_net_change_num_queues(n, max); >>>> >>>> virtio_net_set_queues(n); >>>> } >>>> @@ -1583,21 +1636,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp) >>>> } >>>> >>>> for (i = 0; i < n->max_queues; i++) { >>>> - n->vqs[i].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx); >>>> - if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) { >>>> - n->vqs[i].tx_vq = >>>> - virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer); >>>> - n->vqs[i].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, >>>> - virtio_net_tx_timer, >>>> - &n->vqs[i]); >>>> - } else { >>>> - n->vqs[i].tx_vq = >>>> - virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh); >>>> - n->vqs[i].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[i]); >>>> - } >>>> - >>>> - n->vqs[i].tx_waiting = 0; >>>> - n->vqs[i].n = n; >>>> + virtio_net_add_queue(n, i); >>>> } >>>> >>>> n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl); >>> . >>> >> > > . >