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 E610F1D7 for ; Tue, 5 Dec 2017 15:56:08 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3F95FC014196; Tue, 5 Dec 2017 14:56:08 +0000 (UTC) Received: from [10.36.112.61] (ovpn-112-61.ams2.redhat.com [10.36.112.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5FEC562529; Tue, 5 Dec 2017 14:56:05 +0000 (UTC) To: Laszlo Ersek , 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> <16059002-07f5-26b0-41eb-0808e63882bb@redhat.com> From: Maxime Coquelin Message-ID: Date: Tue, 5 Dec 2017 15:56:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <16059002-07f5-26b0-41eb-0808e63882bb@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 05 Dec 2017 14:56:08 +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:56:09 -0000 Hi Laszlo, On 12/05/2017 03:40 PM, Laszlo Ersek wrote: > 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). Right, thanks for spotting this. I didn't noticed my mistake while testing it because of the NULL check in the loop. > 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". Yes, that sounds cleaner. I think dev->nr_vring can safely be decremented directly, so "i" could be skipped. Thanks! Maxime > Thanks > Laszlo > >> + >> + if (!vq) >> + continue; >> + >> + cleanup_vq(vq, 1); >> + free_vq(vq); >> + dev->nr_vring--; >> + } >> + } >> + >> return 0; >> } >> >> >