| Message ID | 1459988946-5956-1-git-send-email-rich.lane@bigswitch.com (mailing list archive) |
|---|---|
| State | Accepted, 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 3758D2C08; Thu, 7 Apr 2016 02:29:12 +0200 (CEST) Received: from mail-pa0-f48.google.com (mail-pa0-f48.google.com [209.85.220.48]) by dpdk.org (Postfix) with ESMTP id 967712BFA for <dev@dpdk.org>; Thu, 7 Apr 2016 02:29:10 +0200 (CEST) Received: by mail-pa0-f48.google.com with SMTP id zm5so43098538pac.0 for <dev@dpdk.org>; Wed, 06 Apr 2016 17:29:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bigswitch-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id; bh=4xT1XY+DlFcqyfP6XPfOh7JCVbl3OiH0jWY9g0PtNbM=; b=Ow21HoN3jLf/d5mDyw03/iikneIY8aRPkOyk7ZHDoFNYjrsPGjg179uook+0XpjOTS G365YEzHcQgFhlR1W3D3dPbex5AtaBvKMau/SszziyY+jLc2rob0vw8CTnrW8PPG6AaW BMfbEiKVvvColIqh84JNhlC06KuXKSmNStgpkFGcwSjZyf8wctUmql6zVJSEd9SkCUrj 0c8Ir3S++yCkR7LC16aBeN5kVnbCdHCtgzWOMuaS88Q7/CACC7TREcrgOqj5OEepe0Am bkGy+B134F6MzV0R3ZJ2qAhF33zDoUr/0IICtG1utzoBTnODiPPf38ltKbnFDTKXQ0LK XZsw== 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=4xT1XY+DlFcqyfP6XPfOh7JCVbl3OiH0jWY9g0PtNbM=; b=aDNMd3tKcSQqjvqtDNtGw4RnrVjkxM7QfG3OoGwybOK0uoL5XMVMCC7V6NJgWoGDAj BeOoSxMjV+478JFw1X3VLewthYfQuX+gMYfiU3NjfvaYVHyfuyTydtgwDk0N6ZwA4pYK zVhfQMkK+SWdZn7FbaWYuhYK7Cm4kcgT4dXBIf2A5Q7g3PGybhzCF/jYYswXap8kz5A3 DINcw0UKE1HltUXv9Y4TUTIxczgc0iMsF/1P4yJQDydzXbFXc/G4eYwsK6MA5GZ/Ucby Bk0/SMdP9N8UFGyciNU0eIs6vEXCcI5z0Du46yk+4hWXrqx9mrpbtVv7vExqJgVAxOJq 21Jw== X-Gm-Message-State: AD7BkJILVHEBArmshzhIW+QvOBOqcnqqhqF+N65XMAKvTxijX5pdlvLYx3CtVtBcegAJHHNV X-Received: by 10.66.139.137 with SMTP id qy9mr289599pab.57.1459988949919; Wed, 06 Apr 2016 17:29:09 -0700 (PDT) Received: from rlane-work.eng.bigswitch.com (c-67-188-28-208.hsd1.ca.comcast.net. [67.188.28.208]) by smtp.gmail.com with ESMTPSA id t87sm7435829pfa.54.2016.04.06.17.29.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 06 Apr 2016 17:29:09 -0700 (PDT) From: Rich Lane <rich.lane@bigswitch.com> To: dev@dpdk.org Cc: Tetsuya Mukawa <mukawa@igel.co.jp>, Yuanhan Liu <yuanhan.liu@linux.intel.com> Date: Wed, 6 Apr 2016 17:29:06 -0700 Message-Id: <1459988946-5956-1-git-send-email-rich.lane@bigswitch.com> X-Mailer: git-send-email 1.9.1 Subject: [dpdk-dev] [PATCH] vhost: call rte_vhost_enable_guest_notification only on enabled queues 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
Rich Lane
April 7, 2016, 12:29 a.m. UTC
If the vhost PMD were configured with more queues than the guest, the old
code would segfault in rte_vhost_enable_guest_notification due to a NULL
virtqueue pointer.
Fixes: ee584e9710b9 ("vhost: add driver on top of the library")
Signed-off-by: Rich Lane <rich.lane@bigswitch.com>
---
drivers/net/vhost/rte_eth_vhost.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Comments
> > If the vhost PMD were configured with more queues than the guest, the old > code would segfault in rte_vhost_enable_guest_notification due to a NULL > virtqueue pointer. > > Fixes: ee584e9710b9 ("vhost: add driver on top of the library") > Signed-off-by: Rich Lane <rich.lane@bigswitch.com> > --- > drivers/net/vhost/rte_eth_vhost.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/vhost/rte_eth_vhost.c > b/drivers/net/vhost/rte_eth_vhost.c > index b1eb082..310cbef 100644 > --- a/drivers/net/vhost/rte_eth_vhost.c > +++ b/drivers/net/vhost/rte_eth_vhost.c > @@ -265,7 +265,6 @@ new_device(struct virtio_net *dev) > vq->device = dev; > vq->internal = internal; > vq->port = eth_dev->data->port_id; > - rte_vhost_enable_guest_notification(dev, vq- > >virtqueue_id, 0); > } > for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > vq = eth_dev->data->tx_queues[i]; > @@ -274,9 +273,11 @@ new_device(struct virtio_net *dev) > vq->device = dev; > vq->internal = internal; > vq->port = eth_dev->data->port_id; > - rte_vhost_enable_guest_notification(dev, vq- > >virtqueue_id, 0); > } > > + for (i = 0; i < dev->virt_qp_nb * VIRTIO_QNUM; i++) > + rte_vhost_enable_guest_notification(dev, i, 0); > + > dev->flags |= VIRTIO_DEV_RUNNING; > dev->priv = eth_dev; > eth_dev->data->dev_link.link_status = ETH_LINK_UP; > -- > 1.9.1 I see the same issue, and verified that this patch solves it. Thanks! Tested-by: Ciara Loftus <ciara.loftus@intel.com> Thanks, Ciara
Hi, On 4/7/2016 8:29 AM, Rich Lane wrote: > If the vhost PMD were configured with more queues than the guest, the old > code would segfault in rte_vhost_enable_guest_notification due to a NULL > virtqueue pointer. > > Fixes: ee584e9710b9 ("vhost: add driver on top of the library") > Signed-off-by: Rich Lane <rich.lane@bigswitch.com> > --- > drivers/net/vhost/rte_eth_vhost.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c > index b1eb082..310cbef 100644 > --- a/drivers/net/vhost/rte_eth_vhost.c > +++ b/drivers/net/vhost/rte_eth_vhost.c > @@ -265,7 +265,6 @@ new_device(struct virtio_net *dev) > vq->device = dev; > vq->internal = internal; > vq->port = eth_dev->data->port_id; > - rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0); > } > for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > vq = eth_dev->data->tx_queues[i]; > @@ -274,9 +273,11 @@ new_device(struct virtio_net *dev) > vq->device = dev; > vq->internal = internal; > vq->port = eth_dev->data->port_id; > - rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0); > } > > + for (i = 0; i < dev->virt_qp_nb * VIRTIO_QNUM; i++) > + rte_vhost_enable_guest_notification(dev, i, 0); > + > dev->flags |= VIRTIO_DEV_RUNNING; > dev->priv = eth_dev; > eth_dev->data->dev_link.link_status = ETH_LINK_UP; Just one question, when qemu starts a vm, usually, only one queue is enabled, then only 1 tx and 1 rx are called rte_vhost_enable_guest_notification; but after system is up, we use "ethtool -K eth0 combined x" to enable multiqueues, there's no chance to call rte_vhost_enable_guest_notification for other queues, right? Thanks, Jianfeng
> On 4/7/2016 8:29 AM, Rich Lane wrote: > > If the vhost PMD were configured with more queues than the guest, the > old > > code would segfault in rte_vhost_enable_guest_notification due to a NULL > > virtqueue pointer. > > > > Fixes: ee584e9710b9 ("vhost: add driver on top of the library") > > Signed-off-by: Rich Lane <rich.lane@bigswitch.com> > > --- > > drivers/net/vhost/rte_eth_vhost.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/vhost/rte_eth_vhost.c > b/drivers/net/vhost/rte_eth_vhost.c > > index b1eb082..310cbef 100644 > > --- a/drivers/net/vhost/rte_eth_vhost.c > > +++ b/drivers/net/vhost/rte_eth_vhost.c > > @@ -265,7 +265,6 @@ new_device(struct virtio_net *dev) > > vq->device = dev; > > vq->internal = internal; > > vq->port = eth_dev->data->port_id; > > - rte_vhost_enable_guest_notification(dev, vq- > >virtqueue_id, 0); > > } > > for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > > vq = eth_dev->data->tx_queues[i]; > > @@ -274,9 +273,11 @@ new_device(struct virtio_net *dev) > > vq->device = dev; > > vq->internal = internal; > > vq->port = eth_dev->data->port_id; > > - rte_vhost_enable_guest_notification(dev, vq- > >virtqueue_id, 0); > > } > > > > + for (i = 0; i < dev->virt_qp_nb * VIRTIO_QNUM; i++) > > + rte_vhost_enable_guest_notification(dev, i, 0); > > + > > dev->flags |= VIRTIO_DEV_RUNNING; > > dev->priv = eth_dev; > > eth_dev->data->dev_link.link_status = ETH_LINK_UP; > > Just one question, when qemu starts a vm, usually, only one queue is > enabled, then only 1 tx and 1 rx are called > rte_vhost_enable_guest_notification; but after system is up, we use > "ethtool -K eth0 combined x" to enable multiqueues, there's no chance to > call rte_vhost_enable_guest_notification for other queues, right? As far as I know, virt_qp_nb will report the number of queues available, regardless of their state enabled/disabled. So for example if we have 4 queues, but only one enabled, virt_qp_nb should still = 4 and rte_vhost_enable_guest_notification() will be called for all of these queues. Thanks, Ciara > > Thanks, > Jianfeng
On Thu, Apr 07, 2016 at 03:29:32PM +0000, Loftus, Ciara wrote: > > On 4/7/2016 8:29 AM, Rich Lane wrote: > > > If the vhost PMD were configured with more queues than the guest, the > > old > > > code would segfault in rte_vhost_enable_guest_notification due to a NULL > > > virtqueue pointer. > > > > > > Fixes: ee584e9710b9 ("vhost: add driver on top of the library") > > > Signed-off-by: Rich Lane <rich.lane@bigswitch.com> > > > --- > > > drivers/net/vhost/rte_eth_vhost.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/vhost/rte_eth_vhost.c > > b/drivers/net/vhost/rte_eth_vhost.c > > > index b1eb082..310cbef 100644 > > > --- a/drivers/net/vhost/rte_eth_vhost.c > > > +++ b/drivers/net/vhost/rte_eth_vhost.c > > > @@ -265,7 +265,6 @@ new_device(struct virtio_net *dev) > > > vq->device = dev; > > > vq->internal = internal; > > > vq->port = eth_dev->data->port_id; > > > - rte_vhost_enable_guest_notification(dev, vq- > > >virtqueue_id, 0); > > > } > > > for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > > > vq = eth_dev->data->tx_queues[i]; > > > @@ -274,9 +273,11 @@ new_device(struct virtio_net *dev) > > > vq->device = dev; > > > vq->internal = internal; > > > vq->port = eth_dev->data->port_id; > > > - rte_vhost_enable_guest_notification(dev, vq- > > >virtqueue_id, 0); > > > } > > > > > > + for (i = 0; i < dev->virt_qp_nb * VIRTIO_QNUM; i++) > > > + rte_vhost_enable_guest_notification(dev, i, 0); > > > + > > > dev->flags |= VIRTIO_DEV_RUNNING; > > > dev->priv = eth_dev; > > > eth_dev->data->dev_link.link_status = ETH_LINK_UP; > > > > Just one question, when qemu starts a vm, usually, only one queue is > > enabled, then only 1 tx and 1 rx are called > > rte_vhost_enable_guest_notification; but after system is up, we use > > "ethtool -K eth0 combined x" to enable multiqueues, there's no chance to > > call rte_vhost_enable_guest_notification for other queues, right? > > As far as I know, virt_qp_nb will report the number of queues available, regardless of their state enabled/disabled. So for example if we have 4 queues, but only one enabled, virt_qp_nb should still = 4 and rte_vhost_enable_guest_notification() will be called for all of these queues. Yes. --yliu
On Wed, Apr 06, 2016 at 05:29:06PM -0700, Rich Lane wrote: > If the vhost PMD were configured with more queues than the guest, the old > code would segfault in rte_vhost_enable_guest_notification due to a NULL > virtqueue pointer. > > Fixes: ee584e9710b9 ("vhost: add driver on top of the library") > Signed-off-by: Rich Lane <rich.lane@bigswitch.com> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> Thanks. --yliu
On 4/7/2016 11:29 PM, Loftus, Ciara wrote: >> On 4/7/2016 8:29 AM, Rich Lane wrote: >>> If the vhost PMD were configured with more queues than the guest, the >> old >>> code would segfault in rte_vhost_enable_guest_notification due to a NULL >>> virtqueue pointer. >>> >>> Fixes: ee584e9710b9 ("vhost: add driver on top of the library") >>> Signed-off-by: Rich Lane <rich.lane@bigswitch.com> >>> --- >>> drivers/net/vhost/rte_eth_vhost.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/vhost/rte_eth_vhost.c >> b/drivers/net/vhost/rte_eth_vhost.c >>> index b1eb082..310cbef 100644 >>> --- a/drivers/net/vhost/rte_eth_vhost.c >>> +++ b/drivers/net/vhost/rte_eth_vhost.c >>> @@ -265,7 +265,6 @@ new_device(struct virtio_net *dev) >>> vq->device = dev; >>> vq->internal = internal; >>> vq->port = eth_dev->data->port_id; >>> - rte_vhost_enable_guest_notification(dev, vq- >>> virtqueue_id, 0); >>> } >>> for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { >>> vq = eth_dev->data->tx_queues[i]; >>> @@ -274,9 +273,11 @@ new_device(struct virtio_net *dev) >>> vq->device = dev; >>> vq->internal = internal; >>> vq->port = eth_dev->data->port_id; >>> - rte_vhost_enable_guest_notification(dev, vq- >>> virtqueue_id, 0); >>> } >>> >>> + for (i = 0; i < dev->virt_qp_nb * VIRTIO_QNUM; i++) >>> + rte_vhost_enable_guest_notification(dev, i, 0); >>> + >>> dev->flags |= VIRTIO_DEV_RUNNING; >>> dev->priv = eth_dev; >>> eth_dev->data->dev_link.link_status = ETH_LINK_UP; >> Just one question, when qemu starts a vm, usually, only one queue is >> enabled, then only 1 tx and 1 rx are called >> rte_vhost_enable_guest_notification; but after system is up, we use >> "ethtool -K eth0 combined x" to enable multiqueues, there's no chance to >> call rte_vhost_enable_guest_notification for other queues, right? > As far as I know, virt_qp_nb will report the number of queues available, regardless of their state enabled/disabled. So for example if we have 4 queues, but only one enabled, virt_qp_nb should still = 4 and rte_vhost_enable_guest_notification() will be called for all of these queues. Oh, yes. I failed to really get multiqueue configured right, which leads to the wrong conclusion. Sorry. Thanks, Jianfeng > > Thanks, > Ciara > >> Thanks, >> Jianfeng
> > If the vhost PMD were configured with more queues than the guest, the old > > code would segfault in rte_vhost_enable_guest_notification due to a NULL > > virtqueue pointer. > > > > Fixes: ee584e9710b9 ("vhost: add driver on top of the library") > > Signed-off-by: Rich Lane <rich.lane@bigswitch.com> > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> Applied, thanks
On 2016/04/08 2:20, Thomas Monjalon wrote: >>> If the vhost PMD were configured with more queues than the guest, the old >>> code would segfault in rte_vhost_enable_guest_notification due to a NULL >>> virtqueue pointer. >>> >>> Fixes: ee584e9710b9 ("vhost: add driver on top of the library") >>> Signed-off-by: Rich Lane <rich.lane@bigswitch.com> >> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > Applied, thanks Hi Rich and Yuanhan, I am sorry for late reply. I was out of office yesterday. I've tested it also today, and seems to be good. Thanks, Tetsuya
On Fri, Apr 08, 2016 at 10:45:33AM +0900, Tetsuya Mukawa wrote: > On 2016/04/08 2:20, Thomas Monjalon wrote: > >>> If the vhost PMD were configured with more queues than the guest, the old > >>> code would segfault in rte_vhost_enable_guest_notification due to a NULL > >>> virtqueue pointer. > >>> > >>> Fixes: ee584e9710b9 ("vhost: add driver on top of the library") > >>> Signed-off-by: Rich Lane <rich.lane@bigswitch.com> > >> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > Applied, thanks > > Hi Rich and Yuanhan, > > I am sorry for late reply. I was out of office yesterday. Tetsuya, no worry! Man takes leaves, and isn't the reason we have 2 or more maintainers on some blocks? :) --yliu > I've tested it also today, and seems to be good. > > Thanks, > Tetsuya
On 2016/04/08 15:14, Yuanhan Liu wrote: > On Fri, Apr 08, 2016 at 10:45:33AM +0900, Tetsuya Mukawa wrote: >> On 2016/04/08 2:20, Thomas Monjalon wrote: >>>>> If the vhost PMD were configured with more queues than the guest, the old >>>>> code would segfault in rte_vhost_enable_guest_notification due to a NULL >>>>> virtqueue pointer. >>>>> >>>>> Fixes: ee584e9710b9 ("vhost: add driver on top of the library") >>>>> Signed-off-by: Rich Lane <rich.lane@bigswitch.com> >>>> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> >>> Applied, thanks >> Hi Rich and Yuanhan, >> >> I am sorry for late reply. I was out of office yesterday. > Tetsuya, no worry! Man takes leaves, and isn't the reason we have 2 or > more maintainers on some blocks? :) > > --yliu Yeah, thanks! Tetsuya
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c index b1eb082..310cbef 100644 --- a/drivers/net/vhost/rte_eth_vhost.c +++ b/drivers/net/vhost/rte_eth_vhost.c @@ -265,7 +265,6 @@ new_device(struct virtio_net *dev) vq->device = dev; vq->internal = internal; vq->port = eth_dev->data->port_id; - rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0); } for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { vq = eth_dev->data->tx_queues[i]; @@ -274,9 +273,11 @@ new_device(struct virtio_net *dev) vq->device = dev; vq->internal = internal; vq->port = eth_dev->data->port_id; - rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0); } + for (i = 0; i < dev->virt_qp_nb * VIRTIO_QNUM; i++) + rte_vhost_enable_guest_notification(dev, i, 0); + dev->flags |= VIRTIO_DEV_RUNNING; dev->priv = eth_dev; eth_dev->data->dev_link.link_status = ETH_LINK_UP;