| Message ID | 1445932306-11880-1-git-send-email-mukawa@igel.co.jp (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 34A6A6A80; Tue, 27 Oct 2015 08:52:05 +0100 (CET) Received: from mail-pa0-f47.google.com (mail-pa0-f47.google.com [209.85.220.47]) by dpdk.org (Postfix) with ESMTP id CF5706787 for <dev@dpdk.org>; Tue, 27 Oct 2015 08:52:03 +0100 (CET) Received: by pasz6 with SMTP id z6so214644338pas.2 for <dev@dpdk.org>; Tue, 27 Oct 2015 00:52:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=igel_co_jp.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id; bh=cE1R078v41IQJ5c2VZKD/ls6B+iZ/XJYDtNmMyvaKho=; b=hhYpANspDIwTtO3I/ZR2+e7/xNwUjpe37KxU4+Y3rMSImKHv8SUerirmhVgBofUy/W PYgzG2VSuryBJ9UWuQiFW2RGlXjRd4ik1+rZOiKlm/X+NctjPZgbXwUN9wONPqAJwMRe vIN8KFBmS2jGA/9jEu1OiCtFAt9cSpRuBxBoohtrVcrtiixtykml8jrfKiRCRLuv9cVY 0zoKsIw+S02FY3L1EMh+Wukag68EN0hXxIgcEQoE9iacFdJPzio0/09k/EHssugwxaBC UOJcPyz+31zPq42UlfxNWCsWkH2dqeusrFZHsgyhHSGo7BB1oy54a8g/B0AQfhUZvhKF 4Lyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=cE1R078v41IQJ5c2VZKD/ls6B+iZ/XJYDtNmMyvaKho=; b=aWGqgj4Oq0L+f7RclyZs5sbdFg+xWlkWBQw4oZTcun5P5BkY2bLYoqrfoy69e3vivQ S7+qO4JHVbyuVE8RUE0JJjIopFc+hHyNjzz5ZEnFhwgZt0yleRe6djdt+2uSEqKnNbTd zs83MJQf773xGl0idDL8tmTXFyt+5SLBC6lwGOtkv5YPDf8wZg2Xba10n0a4JiguAx1d Bhvip5cM8O1d/tkOiR+KQguXHXrs0E1dty2Uu7WVZ25pFHqIKc+Qbmvs2coGe1fUzO9w R7aT09+2AoGvLBLV37HjcogCu8sgsV6VXxWvylF2bwPIz9y+BOEvqWImYQim8+b6pJpE mfpg== X-Gm-Message-State: ALoCoQmA26we2HVplzwgBdvS+DxvPIGxlJZxq/eiNuUI/bzpoc6dbZHLQH3r3tekFsPo7by8lANb X-Received: by 10.68.131.36 with SMTP id oj4mr26485558pbb.141.1445932323205; Tue, 27 Oct 2015 00:52:03 -0700 (PDT) Received: from localhost.localdomain (napt.igel.co.jp. [219.106.231.132]) by smtp.gmail.com with ESMTPSA id kp4sm37921916pbc.34.2015.10.27.00.52.00 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 27 Oct 2015 00:52:02 -0700 (PDT) From: Tetsuya Mukawa <mukawa@igel.co.jp> To: dev@dpdk.org Date: Tue, 27 Oct 2015 16:51:46 +0900 Message-Id: <1445932306-11880-1-git-send-email-mukawa@igel.co.jp> X-Mailer: git-send-email 2.1.4 Cc: ann.zhuangyanying@huawei.com Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Tetsuya Mukawa
Oct. 27, 2015, 7:51 a.m. UTC
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 <mukawa@igel.co.jp>
---
lib/librte_vhost/vhost_user/virtio-net-user.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
Comments
On Tue, Oct 27, 2015 at 04:51:46PM +0900, 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 <mukawa@igel.co.jp> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> Thanks. --yliu > --- > 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); > > /* 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; > } > > -- > 2.1.4
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 <mukawa@igel.co.jp> > --- > 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 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; > } >
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 <mukawa@igel.co.jp> > > --- > > 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. 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. 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; > > } > > >
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 <mukawa@igel.co.jp> >>> --- >>> 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. > > 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? > 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; >>> } >>>
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 <mukawa@igel.co.jp> > >>> --- > >>> 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; > >>> } > >>> >
On 2015/10/27 17:39, 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 <mukawa@igel.co.jp> >>> --- >>> 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. > > 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. 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. I thought we had 2 choices. 1. Call the callback handler at first place of this function when 1st GET_VRING_BASE message comes. 2. Call the callback handler at last place of this function when 2nd GET_VRING_BASE message comes. And I chose 2nd, because in the case of 1st choice, before sending 2nd message, QEMU guess one of queue is still alive, but actually in DPDK application, it has been closed already. I thought above inconsistency might cause the issue. But yes, if we chose 2nd, we may have an issue as Xie said. > > --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? Yes, I faced illegal memory access. For example, if we have RX and TX queues, we will have 2 GET_VRING_BASE messages when virtio-net device is finalized. While handling these messages, 'dev->virtqueue[2]' will be accessed, then will cause illegal access. (We only have 2 queues, so above will be NULL) So actually we need to change the function a bit. Thanks, Tetsuya
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); /* 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; }