[dpdk-dev] [PATCH 0/2] Fix packed ring header len

[dpdk-dev] [PATCH 0/2] Fix packed ring header len

[Maxime Coquelin]

First patch is fixing packed ring header len. Issue was
reported by Marvin.

Second patch is a (very) light optimization, to make used
directly of the static hdr len value in packed-ring only
functions.

Maxime Coquelin (2):
  vhost: fix Virtio-net header len with packed ring
  vhost: use fixed Virtio-net header len packed ring

 lib/librte_vhost/vhost_user.c |  4 +++-
 lib/librte_vhost/virtio_net.c | 11 ++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.26.2

[dpdk-dev] [PATCH 1/2] vhost: fix Virtio-net header len with packed ring

[Maxime Coquelin]

In case packed ring layout has been negotiated, but neither
Version 1 nor mergeable buffers, the Virtio-net header len
is assigned to the legacy devices value, which is wrong.

This patch fixes this with using the proper len as devices
using packed ring are not legacy devices.

Fixes: a922401f35cc ("vhost: add Rx support for packed ring")
Fixes: ae999ce49dcb ("vhost: add Tx support for packed ring")
Cc: stable@dpdk.org

Reported-by: Marvin Liu <yong.liu@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 4deceb3e00..5d1fb9e863 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -341,7 +341,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 
 	dev->features = features;
 	if (dev->features &
-		((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
+		((1ULL << VIRTIO_NET_F_MRG_RXBUF) |
+		 (1ULL << VIRTIO_F_VERSION_1) |
+		 (1ULL << VIRTIO_F_RING_PACKED))) {
 		dev->vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	} else {
 		dev->vhost_hlen = sizeof(struct virtio_net_hdr);
-- 
2.26.2

Re: [dpdk-dev] [PATCH 1/2] vhost: fix Virtio-net header len with packed ring

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, October 1, 2020 6:12 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Liu, Yong
> <yong.liu@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 1/2] vhost: fix Virtio-net header len with packed ring
> 
> In case packed ring layout has been negotiated, but neither
> Version 1 nor mergeable buffers, the Virtio-net header len
> is assigned to the legacy devices value, which is wrong.
> 
> This patch fixes this with using the proper len as devices
> using packed ring are not legacy devices.
> 
> Fixes: a922401f35cc ("vhost: add Rx support for packed ring")
> Fixes: ae999ce49dcb ("vhost: add Tx support for packed ring")
> Cc: stable@dpdk.org
> 
> Reported-by: Marvin Liu <yong.liu@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost_user.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 4deceb3e00..5d1fb9e863 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -341,7 +341,9 @@ vhost_user_set_features(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
> 
>  	dev->features = features;
>  	if (dev->features &
> -		((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1)))
> {
> +		((1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> +		 (1ULL << VIRTIO_F_VERSION_1) |
> +		 (1ULL << VIRTIO_F_RING_PACKED))) {
>  		dev->vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	} else {
>  		dev->vhost_hlen = sizeof(struct virtio_net_hdr);
> --
> 2.26.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

[dpdk-dev] [PATCH 2/2] vhost: use fixed Virtio-net header len packed ring

[Maxime Coquelin]

This small optimization uses static the Virtio-net header len
in packed datapath, since Virtio-net header cannot be the
legacy one in case of packed ring.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/virtio_net.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 0a0bea1a5a..5865fa5f65 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -1147,7 +1147,7 @@ vhost_enqueue_single_packed(struct virtio_net *dev,
 	uint16_t buf_id = 0;
 	uint32_t len = 0;
 	uint16_t desc_count;
-	uint32_t size = pkt->pkt_len + dev->vhost_hlen;
+	uint32_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	uint16_t num_buffers = 0;
 	uint32_t buffer_len[vq->size];
 	uint16_t buffer_buf_id[vq->size];
@@ -1262,7 +1262,7 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev,
 	uint16_t avail_idx = vq->last_avail_idx;
 	uint64_t desc_addrs[PACKED_BATCH_SIZE];
 	struct virtio_net_hdr_mrg_rxbuf *hdrs[PACKED_BATCH_SIZE];
-	uint32_t buf_offset = dev->vhost_hlen;
+	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	uint64_t lens[PACKED_BATCH_SIZE];
 	uint16_t ids[PACKED_BATCH_SIZE];
 	uint16_t i;
@@ -1308,7 +1308,8 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev,
 		rte_prefetch0((void *)(uintptr_t)desc_addrs[i]);
 		hdrs[i] = (struct virtio_net_hdr_mrg_rxbuf *)
 					(uintptr_t)desc_addrs[i];
-		lens[i] = pkts[i]->pkt_len + dev->vhost_hlen;
+		lens[i] = pkts[i]->pkt_len +
+			sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	}
 
 	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE)
@@ -2277,7 +2278,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net *dev,
 	struct virtio_net_hdr *hdr;
 	uint64_t lens[PACKED_BATCH_SIZE];
 	uint64_t buf_lens[PACKED_BATCH_SIZE];
-	uint32_t buf_offset = dev->vhost_hlen;
+	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	uint16_t flags, i;
 
 	if (unlikely(avail_idx & PACKED_BATCH_MASK))
@@ -2354,7 +2355,7 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev,
 			   struct rte_mbuf **pkts)
 {
 	uint16_t avail_idx = vq->last_avail_idx;
-	uint32_t buf_offset = dev->vhost_hlen;
+	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	uintptr_t desc_addrs[PACKED_BATCH_SIZE];
 	uint16_t ids[PACKED_BATCH_SIZE];
 	uint16_t i;
-- 
2.26.2

Re: [dpdk-dev] [PATCH 2/2] vhost: use fixed Virtio-net header len packed ring

[Xia, Chenbo]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, October 1, 2020 6:12 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Liu, Yong
> <yong.liu@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 2/2] vhost: use fixed Virtio-net header len packed ring
> 
> This small optimization uses static the Virtio-net header len

Better use 'the static Virtio-net header length' here when you apply the
patch 😊. With this fix:

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

> in packed datapath, since Virtio-net header cannot be the
> legacy one in case of packed ring.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/virtio_net.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 0a0bea1a5a..5865fa5f65 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -1147,7 +1147,7 @@ vhost_enqueue_single_packed(struct virtio_net *dev,
>  	uint16_t buf_id = 0;
>  	uint32_t len = 0;
>  	uint16_t desc_count;
> -	uint32_t size = pkt->pkt_len + dev->vhost_hlen;
> +	uint32_t size = pkt->pkt_len + sizeof(struct
> virtio_net_hdr_mrg_rxbuf);
>  	uint16_t num_buffers = 0;
>  	uint32_t buffer_len[vq->size];
>  	uint16_t buffer_buf_id[vq->size];
> @@ -1262,7 +1262,7 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev,
>  	uint16_t avail_idx = vq->last_avail_idx;
>  	uint64_t desc_addrs[PACKED_BATCH_SIZE];
>  	struct virtio_net_hdr_mrg_rxbuf *hdrs[PACKED_BATCH_SIZE];
> -	uint32_t buf_offset = dev->vhost_hlen;
> +	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	uint64_t lens[PACKED_BATCH_SIZE];
>  	uint16_t ids[PACKED_BATCH_SIZE];
>  	uint16_t i;
> @@ -1308,7 +1308,8 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev,
>  		rte_prefetch0((void *)(uintptr_t)desc_addrs[i]);
>  		hdrs[i] = (struct virtio_net_hdr_mrg_rxbuf *)
>  					(uintptr_t)desc_addrs[i];
> -		lens[i] = pkts[i]->pkt_len + dev->vhost_hlen;
> +		lens[i] = pkts[i]->pkt_len +
> +			sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	}
> 
>  	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE)
> @@ -2277,7 +2278,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net
> *dev,
>  	struct virtio_net_hdr *hdr;
>  	uint64_t lens[PACKED_BATCH_SIZE];
>  	uint64_t buf_lens[PACKED_BATCH_SIZE];
> -	uint32_t buf_offset = dev->vhost_hlen;
> +	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	uint16_t flags, i;
> 
>  	if (unlikely(avail_idx & PACKED_BATCH_MASK))
> @@ -2354,7 +2355,7 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev,
>  			   struct rte_mbuf **pkts)
>  {
>  	uint16_t avail_idx = vq->last_avail_idx;
> -	uint32_t buf_offset = dev->vhost_hlen;
> +	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	uintptr_t desc_addrs[PACKED_BATCH_SIZE];
>  	uint16_t ids[PACKED_BATCH_SIZE];
>  	uint16_t i;
> --
> 2.26.2

Re: [dpdk-dev] [PATCH 2/2] vhost: use fixed Virtio-net header len packed ring

[Maxime Coquelin]

Welcome back Chenbo,

On 10/9/20 8:36 AM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, October 1, 2020 6:12 PM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Liu, Yong
>> <yong.liu@intel.com>
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH 2/2] vhost: use fixed Virtio-net header len packed ring
>>
>> This small optimization uses static the Virtio-net header len
> 
> Better use 'the static Virtio-net header length' here when you apply the
> patch 😊. With this fix:

I did the change when applying, thanks!
Maxime

> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> 
>> in packed datapath, since Virtio-net header cannot be the
>> legacy one in case of packed ring.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  lib/librte_vhost/virtio_net.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>> index 0a0bea1a5a..5865fa5f65 100644
>> --- a/lib/librte_vhost/virtio_net.c
>> +++ b/lib/librte_vhost/virtio_net.c
>> @@ -1147,7 +1147,7 @@ vhost_enqueue_single_packed(struct virtio_net *dev,
>>  	uint16_t buf_id = 0;
>>  	uint32_t len = 0;
>>  	uint16_t desc_count;
>> -	uint32_t size = pkt->pkt_len + dev->vhost_hlen;
>> +	uint32_t size = pkt->pkt_len + sizeof(struct
>> virtio_net_hdr_mrg_rxbuf);
>>  	uint16_t num_buffers = 0;
>>  	uint32_t buffer_len[vq->size];
>>  	uint16_t buffer_buf_id[vq->size];
>> @@ -1262,7 +1262,7 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev,
>>  	uint16_t avail_idx = vq->last_avail_idx;
>>  	uint64_t desc_addrs[PACKED_BATCH_SIZE];
>>  	struct virtio_net_hdr_mrg_rxbuf *hdrs[PACKED_BATCH_SIZE];
>> -	uint32_t buf_offset = dev->vhost_hlen;
>> +	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>>  	uint64_t lens[PACKED_BATCH_SIZE];
>>  	uint16_t ids[PACKED_BATCH_SIZE];
>>  	uint16_t i;
>> @@ -1308,7 +1308,8 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev,
>>  		rte_prefetch0((void *)(uintptr_t)desc_addrs[i]);
>>  		hdrs[i] = (struct virtio_net_hdr_mrg_rxbuf *)
>>  					(uintptr_t)desc_addrs[i];
>> -		lens[i] = pkts[i]->pkt_len + dev->vhost_hlen;
>> +		lens[i] = pkts[i]->pkt_len +
>> +			sizeof(struct virtio_net_hdr_mrg_rxbuf);
>>  	}
>>
>>  	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE)
>> @@ -2277,7 +2278,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net
>> *dev,
>>  	struct virtio_net_hdr *hdr;
>>  	uint64_t lens[PACKED_BATCH_SIZE];
>>  	uint64_t buf_lens[PACKED_BATCH_SIZE];
>> -	uint32_t buf_offset = dev->vhost_hlen;
>> +	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>>  	uint16_t flags, i;
>>
>>  	if (unlikely(avail_idx & PACKED_BATCH_MASK))
>> @@ -2354,7 +2355,7 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev,
>>  			   struct rte_mbuf **pkts)
>>  {
>>  	uint16_t avail_idx = vq->last_avail_idx;
>> -	uint32_t buf_offset = dev->vhost_hlen;
>> +	uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>>  	uintptr_t desc_addrs[PACKED_BATCH_SIZE];
>>  	uint16_t ids[PACKED_BATCH_SIZE];
>>  	uint16_t i;
>> --
>> 2.26.2
> 

Re: [dpdk-dev] [PATCH 0/2] Fix packed ring header len

[Maxime Coquelin]

On 10/1/20 12:11 PM, Maxime Coquelin wrote:
> First patch is fixing packed ring header len. Issue was
> reported by Marvin.
> 
> Second patch is a (very) light optimization, to make used
> directly of the static hdr len value in packed-ring only
> functions.
> 
> Maxime Coquelin (2):
>   vhost: fix Virtio-net header len with packed ring
>   vhost: use fixed Virtio-net header len packed ring
> 
>  lib/librte_vhost/vhost_user.c |  4 +++-
>  lib/librte_vhost/virtio_net.c | 11 ++++++-----
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime