[dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Huawei]

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

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Tan, 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

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

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Xie, Huawei]

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
>

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Tan, 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!

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[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>


BTW, I found your email address is abnormal:
    From: Huawei@dpdk.org, Xie@dpdk.org

Something wrong with your git send email config?

	--yliu

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[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?

	--yliu

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Thomas Monjalon]

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?

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Yuanhan Liu]

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

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Xie, Huawei]

> -----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

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Xie, Huawei]

> -----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
> 

Re: [dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Bruce Richardson]

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

[dpdk-dev] [PATCH] virtio: fix virtio_net_hdr desc pointing to the same buffer

[Huawei]

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