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 E725147CE for ; Wed, 13 Dec 2017 09:14:56 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E286F3DE3C; Wed, 13 Dec 2017 08:14:55 +0000 (UTC) Received: from [10.36.112.48] (ovpn-112-48.ams2.redhat.com [10.36.112.48]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D3B225D723; Wed, 13 Dec 2017 08:14:53 +0000 (UTC) To: Tiwei Bie Cc: dev@dpdk.org, yliu@fridaylinux.org, jianfeng.tan@intel.com, lprosek@redhat.com, lersek@redhat.com References: <20171211151503.19195-1-maxime.coquelin@redhat.com> <20171211151503.19195-5-maxime.coquelin@redhat.com> <20171213031615.t7rh2ccjf5gml73h@debian-xvivbkq> From: Maxime Coquelin Message-ID: <63b4063f-1994-8ce2-2682-776e44ee608a@redhat.com> Date: Wed, 13 Dec 2017 09:14:51 +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: <20171213031615.t7rh2ccjf5gml73h@debian-xvivbkq> 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.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Wed, 13 Dec 2017 08:14:55 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v4 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: Wed, 13 Dec 2017 08:14:57 -0000 On 12/13/2017 04:16 AM, Tiwei Bie wrote: > On Mon, Dec 11, 2017 at 04:15:03PM +0100, 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 | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >> index 471b1612c..d5ca1ac90 100644 >> --- a/lib/librte_vhost/vhost_user.c >> +++ b/lib/librte_vhost/vhost_user.c >> @@ -216,6 +216,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. >> + */ >> + while (dev->nr_vring > 2) { >> + struct vhost_virtqueue *vq; >> + >> + vq = dev->virtqueue[--dev->nr_vring]; >> + if (!vq) >> + continue; >> + >> + cleanup_vq(vq, 1); >> + free_vq(vq); > > Hi, > > Sorry, I didn't look into this patch in last version. Don't be sorry, thanks for catching this bug. > The freed dev->virtqueue[$idx] also needs to be zeroed. > Otherwise, it won't be allocated in the future due to the > below check in vhost_user_check_and_alloc_queue_pair(), > and the freed memory will be used again. > > /* > * Allocate a queue pair if it hasn't been allocated yet > */ > static int > vhost_user_check_and_alloc_queue_pair(struct virtio_net *dev, VhostUserMsg *msg) > { > ........ > > if (dev->virtqueue[vring_idx]) > return 0; > > return alloc_vring_queue(dev, vring_idx); > } You are right, I'll post v5 setting dev->virtqueue[$idx] to NULL after free_vq() call. Thanks for the review, Maxime > Best regards, > Tiwei Bie > >> + } >> + } >> + >> return 0; >> } >> >> -- >> 2.14.3 >>