| Message ID | 1449763652-86292-1-git-send-email-huawei.xie@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Bruce Richardson |
| 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 ABD338E80; Fri, 11 Dec 2015 15:22:02 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id A6B458E71 for <dev@dpdk.org>; Fri, 11 Dec 2015 09:02:25 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP; 11 Dec 2015 00:02:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,412,1444719600"; d="scan'208";a="858654592" Received: from dpdk15.sh.intel.com ([10.239.129.25]) by fmsmga001.fm.intel.com with ESMTP; 11 Dec 2015 00:02:23 -0800 From: Huawei@dpdk.org, Xie@dpdk.org To: dev@dpdk.org Date: Fri, 11 Dec 2015 00:07:32 +0800 Message-Id: <1449763652-86292-1-git-send-email-huawei.xie@intel.com> X-Mailer: git-send-email 1.8.1.4 X-Mailman-Approved-At: Fri, 11 Dec 2015 15:22:01 +0100 Subject: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer 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
Huawei@dpdk.org, Xie@dpdk.org
Dec. 10, 2015, 4:07 p.m. UTC
The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause
issue because in the simple TX mode we don't use the header. This patch
makes the header desc point to different buffer.
Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
drivers/net/virtio/virtio_rxtx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei@dpdk.org > Sent: Friday, December 11, 2015 12:08 AM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the > same buffer > > The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause > issue because in the simple TX mode we don't use the header. This patch > makes the header desc point to different buffer. > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > --- > drivers/net/virtio/virtio_rxtx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c > index 74b39ef..6cfd315 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -352,7 +352,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int > queue_type) > vq->vq_ring.desc[i + mid_idx].next = i; > vq->vq_ring.desc[i + mid_idx].addr = > vq->virtio_net_hdr_mem + > - mid_idx * vq->hw- > >vtnet_hdr_size; > + i * vq->hw->vtnet_hdr_size; > vq->vq_ring.desc[i + mid_idx].len = > vq->hw->vtnet_hdr_size; > vq->vq_ring.desc[i + mid_idx].flags = > -- > 1.8.1.4 So in the case when header is not used, shall the way of pointing to all header bufs to one buf help improve the performance? At least, it saves some of cache lines. Thanks, Jianfeng
On 12/12/2015 3:39 PM, Tan, Jianfeng wrote: > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei@dpdk.org >> Sent: Friday, December 11, 2015 12:08 AM >> To: dev@dpdk.org >> Subject: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the >> same buffer >> >> The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause >> issue because in the simple TX mode we don't use the header. This patch >> makes the header desc point to different buffer. >> >> Signed-off-by: Huawei Xie <huawei.xie@intel.com> >> --- >> drivers/net/virtio/virtio_rxtx.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c >> index 74b39ef..6cfd315 100644 >> --- a/drivers/net/virtio/virtio_rxtx.c >> +++ b/drivers/net/virtio/virtio_rxtx.c >> @@ -352,7 +352,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int >> queue_type) >> vq->vq_ring.desc[i + mid_idx].next = i; >> vq->vq_ring.desc[i + mid_idx].addr = >> vq->virtio_net_hdr_mem + >> - mid_idx * vq->hw- >>> vtnet_hdr_size; >> + i * vq->hw->vtnet_hdr_size; >> vq->vq_ring.desc[i + mid_idx].len = >> vq->hw->vtnet_hdr_size; >> vq->vq_ring.desc[i + mid_idx].flags = >> -- >> 1.8.1.4 > So in the case when header is not used, shall the way of pointing to all header bufs to one buf > help improve the performance? At least, it saves some of cache lines. If we don't touch the virtio_net buffer, it doesn't help performance pointing to same buffer. > > Thanks, > Jianfeng >
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei@dpdk.org > Sent: Friday, December 11, 2015 12:08 AM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the > same buffer > > The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause > issue because in the simple TX mode we don't use the header. This patch > makes the header desc point to different buffer. > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > --- > drivers/net/virtio/virtio_rxtx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c > index 74b39ef..6cfd315 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -352,7 +352,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int > queue_type) > vq->vq_ring.desc[i + mid_idx].next = i; > vq->vq_ring.desc[i + mid_idx].addr = > vq->virtio_net_hdr_mem + > - mid_idx * vq->hw- > >vtnet_hdr_size; > + i * vq->hw->vtnet_hdr_size; > vq->vq_ring.desc[i + mid_idx].len = > vq->hw->vtnet_hdr_size; > vq->vq_ring.desc[i + mid_idx].flags = > -- > 1.8.1.4 Acked-by: Jianfeng Tan <Jianfeng.tan@intel.com> Thanks!
On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org wrote: > The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause > issue because in the simple TX mode we don't use the header. This patch > makes the header desc point to different buffer. > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> BTW, I found your email address is abnormal: From: Huawei@dpdk.org, Xie@dpdk.org Something wrong with your git send email config? --yliu
2015-12-14 11:01, Yuanhan Liu: > On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org wrote: > > The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause > > issue because in the simple TX mode we don't use the header. This patch > > makes the header desc point to different buffer. > > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> Does it fix something in the current behaviour? I have the feeling it may wait for 2.3.
On Mon, Dec 14, 2015 at 10:32:24AM +0100, Thomas Monjalon wrote: > 2015-12-14 11:01, Yuanhan Liu: > > On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org wrote: > > > The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause > > > issue because in the simple TX mode we don't use the header. This patch > > > makes the header desc point to different buffer. > > > > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > > > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > Does it fix something in the current behaviour? It's more like a logic fixing to me. > I have the feeling it may wait for 2.3. It's been introduced in v2.2, with Huawei's simple tx patchset. Therefore, I guess 2.2 is good to go? --yliu
2015-12-14 19:47, Yuanhan Liu: > On Mon, Dec 14, 2015 at 10:32:24AM +0100, Thomas Monjalon wrote: > > 2015-12-14 11:01, Yuanhan Liu: > > > On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org wrote: > > > > The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause > > > > issue because in the simple TX mode we don't use the header. This patch > > > > makes the header desc point to different buffer. > > > > > > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > > > > > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > Does it fix something in the current behaviour? > > It's more like a logic fixing to me. > > > I have the feeling it may wait for 2.3. > > It's been introduced in v2.2, with Huawei's simple tx patchset. > Therefore, I guess 2.2 is good to go? The vhost driver has been validated without with patch. Merging it would be taking the risk of breaking something (or just reduce performance) for no clear benefit. Am I missing something?
On Mon, Dec 14, 2015 at 01:44:54PM +0100, Thomas Monjalon wrote: > 2015-12-14 19:47, Yuanhan Liu: > > On Mon, Dec 14, 2015 at 10:32:24AM +0100, Thomas Monjalon wrote: > > > 2015-12-14 11:01, Yuanhan Liu: > > > > On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org wrote: > > > > > The virtio_net_hdr desc all pointed to the same buffer. It doesn't cause > > > > > issue because in the simple TX mode we don't use the header. This patch > > > > > makes the header desc point to different buffer. > > > > > > > > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > > > > > > > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > > Does it fix something in the current behaviour? > > > > It's more like a logic fixing to me. > > > > > I have the feeling it may wait for 2.3. > > > > It's been introduced in v2.2, with Huawei's simple tx patchset. > > Therefore, I guess 2.2 is good to go? > > The vhost driver has been validated without with patch. Huawei stated in the commit log that "It doesn't cause issue because in the simple TX mode we don't use the header". > Merging it would be taking the risk of breaking something > (or just reduce performance) for no clear benefit. > Am I missing something? I know your concerns: we really should be cagy about making any changes when a release is close, especially when all stuff are validated. From this point of view, I agree with you we could delay it to v2.3. Maybe huawei have more inputs here? --yliu
> -----Original Message----- > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > Sent: Monday, December 14, 2015 9:10 PM > To: Thomas Monjalon > Cc: Xie, Huawei; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to > the same buffer > > On Mon, Dec 14, 2015 at 01:44:54PM +0100, Thomas Monjalon wrote: > > 2015-12-14 19:47, Yuanhan Liu: > > > On Mon, Dec 14, 2015 at 10:32:24AM +0100, Thomas Monjalon wrote: > > > > 2015-12-14 11:01, Yuanhan Liu: > > > > > On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org wrote: > > > > > > The virtio_net_hdr desc all pointed to the same buffer. It > doesn't cause > > > > > > issue because in the simple TX mode we don't use the header. This > patch > > > > > > makes the header desc point to different buffer. > > > > > > > > > > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > > > > > > > > > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > > > > Does it fix something in the current behaviour? > > > > > > It's more like a logic fixing to me. > > > > > > > I have the feeling it may wait for 2.3. > > > > > > It's been introduced in v2.2, with Huawei's simple tx patchset. > > > Therefore, I guess 2.2 is good to go? > > > > The vhost driver has been validated without with patch. > > Huawei stated in the commit log that "It doesn't cause issue because in > the simple TX mode we don't use the header". > > > Merging it would be taking the risk of breaking something > > (or just reduce performance) for no clear benefit. > > Am I missing something? > Thomas, there is no risk at all with this patch, and it will not affect performance. I prefer to integrate this patch, so that we have a good looking vhost library. :). > I know your concerns: we really should be cagy about making any changes > when a release is close, especially when all stuff are validated. From > this point of view, I agree with you we could delay it to v2.3. > > Maybe huawei have more inputs here? > > --yliu
2015-12-14 13:38, Xie, Huawei: > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > > On Mon, Dec 14, 2015 at 01:44:54PM +0100, Thomas Monjalon wrote: > > > 2015-12-14 19:47, Yuanhan Liu: > > > > On Mon, Dec 14, 2015 at 10:32:24AM +0100, Thomas Monjalon wrote: > > > > > 2015-12-14 11:01, Yuanhan Liu: > > > > > > On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org wrote: > > > > > > > The virtio_net_hdr desc all pointed to the same buffer. It > > doesn't cause > > > > > > > issue because in the simple TX mode we don't use the header. This > > patch > > > > > > > makes the header desc point to different buffer. > > > > > > > > > > > > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > > > > > > > > > > > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > > > > > > Does it fix something in the current behaviour? > > > > > > > > It's more like a logic fixing to me. > > > > > > > > > I have the feeling it may wait for 2.3. > > > > > > > > It's been introduced in v2.2, with Huawei's simple tx patchset. > > > > Therefore, I guess 2.2 is good to go? > > > > > > The vhost driver has been validated without with patch. > > > > Huawei stated in the commit log that "It doesn't cause issue because in > > the simple TX mode we don't use the header". > > > > > Merging it would be taking the risk of breaking something > > > (or just reduce performance) for no clear benefit. > > > Am I missing something? > > > Thomas, there is no risk at all with this patch, and it will not affect performance. > I prefer to integrate this patch, so that we have a good looking vhost library. :). I'm not sure that "good looking" is enough. I'll wait for another opinion before merging, so we'll have 2 responsibles in case of failure :) > > I know your concerns: we really should be cagy about making any changes > > when a release is close, especially when all stuff are validated. From > > this point of view, I agree with you we could delay it to v2.3. > > > > Maybe huawei have more inputs here? > > > > --yliu
> -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Monday, December 14, 2015 9:45 PM > To: Xie, Huawei > Cc: Yuanhan Liu; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc > pointing to the same buffer > > 2015-12-14 13:38, Xie, Huawei: > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > > > On Mon, Dec 14, 2015 at 01:44:54PM +0100, Thomas Monjalon wrote: > > > > 2015-12-14 19:47, Yuanhan Liu: > > > > > On Mon, Dec 14, 2015 at 10:32:24AM +0100, Thomas Monjalon > wrote: > > > > > > 2015-12-14 11:01, Yuanhan Liu: > > > > > > > On Fri, Dec 11, 2015 at 12:07:32AM +0800, Huawei@dpdk.org > wrote: > > > > > > > > The virtio_net_hdr desc all pointed to the same buffer. > It > > > doesn't cause > > > > > > > > issue because in the simple TX mode we don't use the > header. This > > > patch > > > > > > > > makes the header desc point to different buffer. > > > > > > > > > > > > > > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com> > > > > > > > > > > > > > > Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > > > > > > > > Does it fix something in the current behaviour? > > > > > > > > > > It's more like a logic fixing to me. > > > > > > > > > > > I have the feeling it may wait for 2.3. > > > > > > > > > > It's been introduced in v2.2, with Huawei's simple tx patchset. > > > > > Therefore, I guess 2.2 is good to go? > > > > > > > > The vhost driver has been validated without with patch. > > > > > > Huawei stated in the commit log that "It doesn't cause issue > because in > > > the simple TX mode we don't use the header". > > > > > > > Merging it would be taking the risk of breaking something > > > > (or just reduce performance) for no clear benefit. > > > > Am I missing something? > > > > > Thomas, there is no risk at all with this patch, and it will not > affect performance. > > I prefer to integrate this patch, so that we have a good looking > vhost library. :). > > I'm not sure that "good looking" is enough. > I'll wait for another opinion before merging, so we'll have 2 > responsibles > in case of failure :) Np. There is no issue either apply this patch or delay it to 2.3. > > > > I know your concerns: we really should be cagy about making any > changes > > > when a release is close, especially when all stuff are validated. > From > > > this point of view, I agree with you we could delay it to v2.3. > > > > > > Maybe huawei have more inputs here? > > > > > > --yliu >
On Mon, Dec 14, 2015 at 01:58:34PM +0000, Xie, Huawei wrote: > > > > -----Original Message----- > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > Sent: Monday, December 14, 2015 9:45 PM > > To: Xie, Huawei > > Cc: Yuanhan Liu; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc > > pointing to the same buffer > > <snip> > > 2015-12-14 13:38, Xie, Huawei: > Np. There is no issue either apply this patch or delay it to 2.3. > Patch now applied to dpdk-next-net/rel_16_04. /Bruce
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c index 74b39ef..6cfd315 100644 --- a/drivers/net/virtio/virtio_rxtx.c +++ b/drivers/net/virtio/virtio_rxtx.c @@ -352,7 +352,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type) vq->vq_ring.desc[i + mid_idx].next = i; vq->vq_ring.desc[i + mid_idx].addr = vq->virtio_net_hdr_mem + - mid_idx * vq->hw->vtnet_hdr_size; + i * vq->hw->vtnet_hdr_size; vq->vq_ring.desc[i + mid_idx].len = vq->hw->vtnet_hdr_size; vq->vq_ring.desc[i + mid_idx].flags =