From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id E67042A5B for ; Tue, 27 Oct 2015 09:24:04 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 27 Oct 2015 01:24:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,204,1444719600"; d="scan'208";a="672524789" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 27 Oct 2015 01:24:04 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 27 Oct 2015 01:24:03 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 27 Oct 2015 01:24:02 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.96]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.204]) with mapi id 14.03.0248.002; Tue, 27 Oct 2015 16:24:01 +0800 From: "Xie, Huawei" To: Tetsuya Mukawa , "dev@dpdk.org" Thread-Topic: [PATCH] vhost: Fix wrong handling of virtqueue array index Thread-Index: AdEQkNSvJJMvVtvXSCeGqd2h22gBFA== Date: Tue, 27 Oct 2015 08:24:00 +0000 Message-ID: References: <1445932306-11880-1-git-send-email-mukawa@igel.co.jp> 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: "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:24:05 -0000 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_v= host/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= 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=