From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id EE70C374E for ; Tue, 27 Oct 2015 09:56:15 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 27 Oct 2015 01:56:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,204,1444719600"; d="scan'208";a="588766919" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by FMSMGA003.fm.intel.com with ESMTP; 27 Oct 2015 01:56:13 -0700 Date: Tue, 27 Oct 2015 16:57:32 +0800 From: Yuanhan Liu To: "Xie, Huawei" Message-ID: <20151027085732.GH3115@yliu-dev.sh.intel.com> References: <1445932306-11880-1-git-send-email-mukawa@igel.co.jp> <20151027083957.GG3115@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Cc: "dev@dpdk.org" , "ann.zhuangyanying@huawei.com" Subject: Re: [dpdk-dev] [PATCH] vhost: Fix wrong handling of virtqueue array index X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Oct 2015 08:56:16 -0000 On Tue, Oct 27, 2015 at 08:46:48AM +0000, Xie, Huawei wrote: > On 10/27/2015 4:39 PM, Yuanhan Liu wrote: > > On Tue, Oct 27, 2015 at 08:24:00AM +0000, Xie, Huawei wrote: > >> On 10/27/2015 3:52 PM, Tetsuya Mukawa wrote: > >>> The patch fixes wrong handling of virtqueue array index when > >>> GET_VRING_BASE message comes. > >>> The vhost backend will receive the message per virtqueue. > >>> Also we should call a destroy callback handler when both RXQ > >>> and TXQ receives the message. > >>> > >>> Signed-off-by: Tetsuya Mukawa > >>> --- > >>> lib/librte_vhost/vhost_user/virtio-net-user.c | 20 ++++++++++---------- > >>> 1 file changed, 10 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c b/lib/librte_vhost/vhost_user/virtio-net-user.c > >>> index a998ad8..99c075f 100644 > >>> --- a/lib/librte_vhost/vhost_user/virtio-net-user.c > >>> +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c > >>> @@ -283,12 +283,10 @@ user_get_vring_base(struct vhost_device_ctx ctx, > >>> struct vhost_vring_state *state) > >>> { > >>> struct virtio_net *dev = get_device(ctx); > >>> + uint16_t base_idx = state->index / VIRTIO_QNUM * VIRTIO_QNUM; > >>> > >>> if (dev == NULL) > >>> return -1; > >>> - /* We have to stop the queue (virtio) if it is running. */ > >>> - if (dev->flags & VIRTIO_DEV_RUNNING) > >>> - notify_ops->destroy_device(dev); > >> Hi Tetsuya: > >> I don't understand why we move it to the end of the function. > >> If we don't tell the application to remove the virtio device from the > > As you stated, he just moved it to the end of the function: it > > still does invoke notfiy_ops->destroy_device() in the end. > The problem is before calling destroy_device, we shouldn't modify the > virtio_net data structure as data plane is also using it. Right then, we may shoud not move it in the end. > > > > And the reason he moved it to the end is he want to invoke the > > callback just when the second GET_VRING_BASE message is received > > for the queue pair. > Don't get it. What issue it fixes? I guess Tetsuya thinks that'd be a more proper time to invoke the callback, but in fact, it's not, as we have MQ enabled :) --yliu > > And while thinking twice, it's not necessary, > > as we will do the "flags & VIRTIO_DEV_RUNNING" check first, it > > doesn't matter on which virt queue we invoke the callback. > > > > > > --yliu > > > >> data plane, then the vhost application is still operating on that > >> device, we shouldn't do anything to the virtio_net device. > >> For this case, as vhost doesn't use kickfd, it will not cause issue, but > >> i think it is best practice firstly to remove it from data plan through > >> destroy_device. > >> > >> I think we could call destroy_device the first time we receive this > >> message. Currently we don't have per queue granularity control to only > >> remove one queue from data plane. > >> > >> I am Okay to only close the kickfd for the specified queue index. > >> > >> Btw, do you meet issue with previous implementation? > >>> > >>> /* Here we are safe to get the last used index */ > >>> ops->get_vring_base(ctx, state->index, state); > >>> @@ -300,15 +298,17 @@ user_get_vring_base(struct vhost_device_ctx ctx, > >>> * sent and only sent in vhost_vring_stop. > >>> * TODO: cleanup the vring, it isn't usable since here. > >>> */ > >>> - if (dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd >= 0) { > >>> - close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd); > >>> - dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd = -1; > >>> - } > >>> - if (dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd >= 0) { > >>> - close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd); > >>> - dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd = -1; > >>> + if (dev->virtqueue[state->index]->kickfd >= 0) { > >>> + close(dev->virtqueue[state->index]->kickfd); > >>> + dev->virtqueue[state->index]->kickfd = -1; > >>> } > >>> > >>> + /* We have to stop the queue (virtio) if it is running. */ > >>> + if ((dev->flags & VIRTIO_DEV_RUNNING) && > >>> + (dev->virtqueue[base_idx + VIRTIO_RXQ]->kickfd == -1) && > >>> + (dev->virtqueue[base_idx + VIRTIO_TXQ]->kickfd == -1)) > >>> + notify_ops->destroy_device(dev); > >>> + > >>> return 0; > >>> } > >>> >