From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id B1D092A5B for ; Tue, 27 Oct 2015 09:46:52 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP; 27 Oct 2015 01:46:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,204,1444719600"; d="scan'208";a="804084472" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 27 Oct 2015 01:46:51 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 27 Oct 2015 01:46:51 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 27 Oct 2015 01:46:50 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.96]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.106]) with mapi id 14.03.0248.002; Tue, 27 Oct 2015 16:46:49 +0800 From: "Xie, Huawei" To: Yuanhan Liu Thread-Topic: [dpdk-dev] [PATCH] vhost: Fix wrong handling of virtqueue array index Thread-Index: AdEQkNSvJJMvVtvXSCeGqd2h22gBFA== Date: Tue, 27 Oct 2015 08:46:48 +0000 Message-ID: References: <1445932306-11880-1-git-send-email-mukawa@igel.co.jp> <20151027083957.GG3115@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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:46:53 -0000 On 10/27/2015 4:39 PM, Yuanhan Liu wrote:=0A= > On Tue, Oct 27, 2015 at 08:24:00AM +0000, Xie, Huawei wrote:=0A= >> On 10/27/2015 3:52 PM, Tetsuya Mukawa wrote:=0A= >>> The patch fixes wrong handling of virtqueue array index when=0A= >>> GET_VRING_BASE message comes.=0A= >>> The vhost backend will receive the message per virtqueue.=0A= >>> Also we should call a destroy callback handler when both RXQ=0A= >>> and TXQ receives the message.=0A= >>>=0A= >>> Signed-off-by: Tetsuya Mukawa =0A= >>> ---=0A= >>> lib/librte_vhost/vhost_user/virtio-net-user.c | 20 ++++++++++---------= -=0A= >>> 1 file changed, 10 insertions(+), 10 deletions(-)=0A= >>>=0A= >>> diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c b/lib/librte= _vhost/vhost_user/virtio-net-user.c=0A= >>> index a998ad8..99c075f 100644=0A= >>> --- a/lib/librte_vhost/vhost_user/virtio-net-user.c=0A= >>> +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c=0A= >>> @@ -283,12 +283,10 @@ user_get_vring_base(struct vhost_device_ctx ctx,= =0A= >>> struct vhost_vring_state *state)=0A= >>> {=0A= >>> struct virtio_net *dev =3D get_device(ctx);=0A= >>> + uint16_t base_idx =3D state->index / VIRTIO_QNUM * VIRTIO_QNUM;=0A= >>> =0A= >>> if (dev =3D=3D NULL)=0A= >>> return -1;=0A= >>> - /* We have to stop the queue (virtio) if it is running. */=0A= >>> - if (dev->flags & VIRTIO_DEV_RUNNING)=0A= >>> - notify_ops->destroy_device(dev);=0A= >> Hi Tetsuya:=0A= >> I don't understand why we move it to the end of the function.=0A= >> If we don't tell the application to remove the virtio device from the=0A= > As you stated, he just moved it to the end of the function: it=0A= > still does invoke notfiy_ops->destroy_device() in the end.=0A= The problem is before calling destroy_device, we shouldn't modify the=0A= virtio_net data structure as data plane is also using it.=0A= >=0A= > And the reason he moved it to the end is he want to invoke the=0A= > callback just when the second GET_VRING_BASE message is received=0A= > for the queue pair.=0A= Don't get it. What issue it fixes?=0A= > And while thinking twice, it's not necessary,=0A= > as we will do the "flags & VIRTIO_DEV_RUNNING" check first, it=0A= > doesn't matter on which virt queue we invoke the callback.=0A= >=0A= >=0A= > --yliu=0A= >=0A= >> data plane, then the vhost application is still operating on that=0A= >> device, we shouldn't do anything to the virtio_net device.=0A= >> For this case, as vhost doesn't use kickfd, it will not cause issue, but= =0A= >> i think it is best practice firstly to remove it from data plan through= =0A= >> destroy_device.=0A= >>=0A= >> I think we could call destroy_device the first time we receive this=0A= >> message. Currently we don't have per queue granularity control to only= =0A= >> remove one queue from data plane.=0A= >>=0A= >> I am Okay to only close the kickfd for the specified queue index.=0A= >>=0A= >> Btw, do you meet issue with previous implementation?=0A= >>> =0A= >>> /* Here we are safe to get the last used index */=0A= >>> ops->get_vring_base(ctx, state->index, state);=0A= >>> @@ -300,15 +298,17 @@ user_get_vring_base(struct vhost_device_ctx ctx,= =0A= >>> * sent and only sent in vhost_vring_stop.=0A= >>> * TODO: cleanup the vring, it isn't usable since here.=0A= >>> */=0A= >>> - if (dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd >=3D 0) {=0A= >>> - close(dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd);=0A= >>> - dev->virtqueue[state->index + VIRTIO_RXQ]->kickfd =3D -1;=0A= >>> - }=0A= >>> - if (dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd >=3D 0) {=0A= >>> - close(dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd);=0A= >>> - dev->virtqueue[state->index + VIRTIO_TXQ]->kickfd =3D -1;=0A= >>> + if (dev->virtqueue[state->index]->kickfd >=3D 0) {=0A= >>> + close(dev->virtqueue[state->index]->kickfd);=0A= >>> + dev->virtqueue[state->index]->kickfd =3D -1;=0A= >>> }=0A= >>> =0A= >>> + /* We have to stop the queue (virtio) if it is running. */=0A= >>> + if ((dev->flags & VIRTIO_DEV_RUNNING) &&=0A= >>> + (dev->virtqueue[base_idx + VIRTIO_RXQ]->kickfd =3D=3D -1) &&=0A= >>> + (dev->virtqueue[base_idx + VIRTIO_TXQ]->kickfd =3D=3D -1))=0A= >>> + notify_ops->destroy_device(dev);=0A= >>> +=0A= >>> return 0;=0A= >>> }=0A= >>> =0A= =0A=