[dpdk-dev] [PATCH 0/4] vhost: Fix various coverity issues in vhost library

[dpdk-dev] [PATCH 0/4] vhost: Fix various coverity issues in vhost library

[Huawei]

Huawei Xie (4):
  vhost: Fix Coverity issue with possible array out-of-bounds read
  vhost: Fix Coverity issue with missed break in switch
  vhost: Fix Coverity issue with missed unlocking
  vhost: Fix Coverity issue with logically dead code

 lib/librte_vhost/vhost_user/fd_man.c         | 4 +++-
 lib/librte_vhost/vhost_user/vhost-net-user.c | 4 +++-
 lib/librte_vhost/virtio-net.c                | 3 +--
 3 files changed, 7 insertions(+), 4 deletions(-)

-- 
1.8.1.4

[dpdk-dev] [PATCH 1/4] vhost: Fix Coverity issue with possible array out-of-bounds read

[Huawei]

CID 107126 (#1 OF 1): Out-of-bounds read
Fixes: 8f972312b8f4 ("vhost: support vhost-user")

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/vhost_user/vhost-net-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index 2dc0547..549f907 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -333,7 +333,7 @@ vserver_message_handler(int connfd, void *dat, int *remove)
 
 	ctx.fh = cfd_ctx->fh;
 	ret = read_vhost_message(connfd, &msg);
-	if (ret <= 0 || msg.request > VHOST_USER_MAX) {
+	if (ret <= 0 || msg.request >= VHOST_USER_MAX) {
 		if (ret < 0)
 			RTE_LOG(ERR, VHOST_CONFIG,
 				"vhost read message failed\n");
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 1/4] vhost: Fix Coverity issue with possible array out-of-bounds read

[Mcnamara, John]

> -----Original Message-----
> From: Huawei Xie
> Sent: Thursday, December 10, 2015 5:57 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John; Xie, Huawei
> Subject: [PATCH 1/4] vhost: Fix Coverity issue with possible array out-of-
> bounds read
> 
> CID 107126 (#1 OF 1): Out-of-bounds read
> Fixes: 8f972312b8f4 ("vhost: support vhost-user")
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Thanks. In future could you also update the Coverity Action to "Fix Submitted".
I'll do it this time for the ones I ack. 

Acked-by: John McNamara <john.mcnamara@intel.com>

Re: [dpdk-dev] [PATCH 1/4] vhost: Fix Coverity issue with possible array out-of-bounds read

[Xie, Huawei]

On 12/11/2015 10:49 PM, Mcnamara, John wrote:
>> -----Original Message-----
>> From: Huawei Xie
>> Sent: Thursday, December 10, 2015 5:57 PM
>> To: dev@dpdk.org
>> Cc: Mcnamara, John; Xie, Huawei
>> Subject: [PATCH 1/4] vhost: Fix Coverity issue with possible array out-of-
>> bounds read
>>
>> CID 107126 (#1 OF 1): Out-of-bounds read
>> Fixes: 8f972312b8f4 ("vhost: support vhost-user")
>>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Thanks. In future could you also update the Coverity Action to "Fix Submitted".
> I'll do it this time for the ones I ack. 
Thanks John. I already planned to do this.
>
> Acked-by: John McNamara <john.mcnamara@intel.com>
>
>

[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

[dpdk-dev] [PATCH 2/4] vhost: Fix Coverity issue with missed break in switch

[Huawei]

CID 107114 (#1 of 1): Missing break in switch
Fixes: 8f972312b8f4 ("vhost: support vhost-user")

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/vhost_user/vhost-net-user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index 549f907..8b7a448 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -389,6 +389,8 @@ vserver_message_handler(int connfd, void *dat, int *remove)
 
 	case VHOST_USER_SET_LOG_BASE:
 		RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
+		break;
+
 	case VHOST_USER_SET_LOG_FD:
 		close(msg.fds[0]);
 		RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 2/4] vhost: Fix Coverity issue with missed break in switch

[Mcnamara, John]

> -----Original Message-----
> From: Huawei Xie
> Sent: Thursday, December 10, 2015 5:57 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John; Xie, Huawei
> Subject: [PATCH 2/4] vhost: Fix Coverity issue with missed break in switch
> 
> CID 107114 (#1 of 1): Missing break in switch
> Fixes: 8f972312b8f4 ("vhost: support vhost-user")
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Acked-by: John McNamara <john.mcnamara@intel.com>

[dpdk-dev] [PATCH 3/4] vhost: Fix Coverity issue with missed unlocking

[Huawei]

CID 107113 (#1 of 1): Missing unlock (LOCK)5. missing_unlock: Returning
without unlocking pfdset->fd_mutex.
Fixes: fbf7e07ca142 ("vhost: add select based event driven processing")

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/vhost_user/fd_man.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
index d68b270..087aaed 100644
--- a/lib/librte_vhost/vhost_user/fd_man.c
+++ b/lib/librte_vhost/vhost_user/fd_man.c
@@ -150,8 +150,10 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
 
 	/* Find a free slot in the list. */
 	i = fdset_find_free_slot(pfdset);
-	if (i == -1)
+	if (i == -1) {
+		pthread_mutex_unlock(&pfdset->fd_mutex);
 		return -2;
+	}
 
 	fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
 	pfdset->num++;
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 3/4] vhost: Fix Coverity issue with missed unlocking

[Mcnamara, John]

> -----Original Message-----
> From: Huawei Xie
> Sent: Thursday, December 10, 2015 5:57 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John; Xie, Huawei
> Subject: [PATCH 3/4] vhost: Fix Coverity issue with missed unlocking
> 
> CID 107113 (#1 of 1): Missing unlock (LOCK)5. missing_unlock: Returning
> without unlocking pfdset->fd_mutex.
> Fixes: fbf7e07ca142 ("vhost: add select based event driven processing")
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Acked-by: John McNamara <john.mcnamara@intel.com>

[dpdk-dev] [PATCH 4/4] vhost: Fix Coverity issue with logically dead code

[Huawei]

CID 107107 (#1 of 1): Logically dead code
Fixes: af4f2c5feb2e ("vhost: fix code style")

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/virtio-net.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 8364938..de78a0f 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -863,8 +863,7 @@ int rte_vhost_enable_guest_notification(struct virtio_net *dev,
 		return -1;
 	}
 
-	dev->virtqueue[queue_id]->used->flags =
-		enable ? 0 : VRING_USED_F_NO_NOTIFY;
+	dev->virtqueue[queue_id]->used->flags = VRING_USED_F_NO_NOTIFY;
 	return 0;
 }
 
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 4/4] vhost: Fix Coverity issue with logically dead code

[Mcnamara, John]

> -----Original Message-----
> From: Huawei Xie
> Sent: Thursday, December 10, 2015 5:57 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John; Xie, Huawei
> Subject: [PATCH 4/4] vhost: Fix Coverity issue with logically dead code
> 
> CID 107107 (#1 of 1): Logically dead code
> Fixes: af4f2c5feb2e ("vhost: fix code style")
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Acked-by: John McNamara <john.mcnamara@intel.com>

Re: [dpdk-dev] [PATCH 0/4] vhost: Fix various coverity issues in vhost library

[Thomas Monjalon]

2015-12-11 01:57, Huawei@dpdk.org, Xie@dpdk.org:
> Huawei Xie (4):
>   vhost: Fix Coverity issue with possible array out-of-bounds read
>   vhost: Fix Coverity issue with missed break in switch
>   vhost: Fix Coverity issue with missed unlocking
>   vhost: Fix Coverity issue with logically dead code

Simple fixes.
Applied, thanks

[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