From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id B05A31D7 for ; Tue, 5 Dec 2017 15:41:11 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D6EA883B6; Tue, 5 Dec 2017 14:41:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-60.rdu2.redhat.com [10.10.123.60]) by smtp.corp.redhat.com (Postfix) with ESMTP id C4AF97838B; Tue, 5 Dec 2017 14:40:56 +0000 (UTC) To: Maxime Coquelin , dev@dpdk.org, yliu@fridaylinux.org, tiwei.bie@intel.com, jianfeng.tan@intel.com Cc: lprosek@redhat.com References: <20171205083434.14292-1-maxime.coquelin@redhat.com> <20171205083434.14292-5-maxime.coquelin@redhat.com> From: Laszlo Ersek Message-ID: <16059002-07f5-26b0-41eb-0808e63882bb@redhat.com> Date: Tue, 5 Dec 2017 15:40:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171205083434.14292-5-maxime.coquelin@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 05 Dec 2017 14:41:11 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v2 4/4] vhost: destroy unused virtqueues when multiqueue not negotiated X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Dec 2017 14:41:12 -0000 Hi Maxime, On 12/05/17 09:34, Maxime Coquelin wrote: > QEMU sends VHOST_USER_SET_VRING_CALL requests for all queues > declared in QEMU command line before the guest is started. > It has the effect in DPDK vhost-user backend to allocate vrings > for all queues declared by QEMU. > > If the first driver being used does not support multiqueue, > the device never changes to VIRTIO_DEV_RUNNING state as only > the first queue pair is initialized. One driver impacted by > this bug is virtio-net's iPXE driver which does not support > VIRTIO_NET_F_MQ feature. > > It is safe to destroy unused virtqueues in SET_FEATURES request > handler, as it is ensured the device is not in running state > at this stage, so virtqueues aren't being processed. > > Signed-off-by: Maxime Coquelin > --- > lib/librte_vhost/vhost_user.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index a5e1f2482..b17080215 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -173,6 +173,7 @@ vhost_user_get_features(struct virtio_net *dev) > static int > vhost_user_set_features(struct virtio_net *dev, uint64_t features) > { > + int i; > uint64_t vhost_features = 0; > > rte_vhost_driver_get_features(dev->ifname, &vhost_features); > @@ -216,6 +217,24 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features) > (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off", > (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off"); > > + if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) { > + /* > + * Remove all but first queue pair if MQ hasn't been > + * negotiated. This is safe because the device is not > + * running at this stage. > + */ > + for (i = dev->nr_vring; i > 1; i--) { > + struct vhost_virtqueue *vq = dev->virtqueue[i]; Sorry, I don't have any experience with dpdk. But, if "dev->virtqueue" has "dev->nr_vring" elements, then this loop is off-by one; dev->virtqueue[dev->nr_vring] should never be accessed. For example, if you have three queues, numbered 0, 1 and 2, this loop will access/release virtqueue[3] (bad) and virtqueue[2] (good). Instead, I suggest: i = dev->nr_vring; while (i > 2) { struct vhost_virtqueue *vq; vq = dev->virtqueue[--i]; /* the rest here */ } The loop body is entered with "i" standing for "how many queues are left that should be freed". Thanks Laszlo > + > + if (!vq) > + continue; > + > + cleanup_vq(vq, 1); > + free_vq(vq); > + dev->nr_vring--; > + } > + } > + > return 0; > } > >