[PATCH] net/nfp: fix issue of data len exceeds descriptor limitation

[PATCH] net/nfp: fix issue of data len exceeds descriptor limitation

[Chaoyong He]

From: Long Wu <long.wu@corigine.com>

If dma_len is larger than "NFDK_DESC_TX_DMA_LEN_HEAD", the value of
dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD maybe less than packet
head length. Fill maximum dma_len in first tx descriptor to make
sure the whole head is included in the first descriptor. In addition,
add some explanation for NFDK code more readable.

Fixes: c73dced48c8c ("net/nfp: add NFDk Tx")
Cc: jin.liu@corigine.com
Cc: stable@dpdk.org

Signed-off-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/nfp_rxtx.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
index b8c874d315..ed88d740fa 100644
--- a/drivers/net/nfp/nfp_rxtx.c
+++ b/drivers/net/nfp/nfp_rxtx.c
@@ -1064,6 +1064,7 @@ nfp_net_nfdk_tx_maybe_close_block(struct nfp_net_txq *txq, struct rte_mbuf *pkt)
 	if (unlikely(n_descs > NFDK_TX_DESC_GATHER_MAX))
 		return -EINVAL;
 
+	/* Under count by 1 (don't count meta) for the round down to work out */
 	n_descs += !!(pkt->ol_flags & RTE_MBUF_F_TX_TCP_SEG);
 
 	if (round_down(txq->wr_p, NFDK_TX_DESC_BLOCK_CNT) !=
@@ -1180,6 +1181,7 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 	/* Sending packets */
 	while ((npkts < nb_pkts) && free_descs) {
 		uint32_t type, dma_len, dlen_type, tmp_dlen;
+		uint32_t tmp_hlen;
 		int nop_descs, used_descs;
 
 		pkt = *(tx_pkts + npkts);
@@ -1218,8 +1220,23 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 		} else {
 			type = NFDK_DESC_TX_TYPE_GATHER;
 		}
+
+		/* Implicitly truncates to chunk in below logic */
 		dma_len -= 1;
-		dlen_type = (NFDK_DESC_TX_DMA_LEN_HEAD & dma_len) |
+
+		/*
+		 * We will do our best to pass as much data as we can in descriptor
+		 * and we need to make sure the first descriptor includes whole
+		 * head since there is limitation in firmware side. Sometimes the
+		 * value of dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD will less
+		 * than packet head len.
+		 */
+		if (dma_len > NFDK_DESC_TX_DMA_LEN_HEAD)
+			tmp_hlen = NFDK_DESC_TX_DMA_LEN_HEAD;
+		else
+			tmp_hlen = dma_len;
+
+		dlen_type = (NFDK_DESC_TX_DMA_LEN_HEAD & tmp_hlen) |
 			(NFDK_DESC_TX_TYPE_HEAD & (type << 12));
 		ktxds->dma_len_type = rte_cpu_to_le_16(dlen_type);
 		dma_addr = rte_mbuf_data_iova(pkt);
@@ -1229,10 +1246,18 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 		ktxds->dma_addr_lo = rte_cpu_to_le_32(dma_addr & 0xffffffff);
 		ktxds++;
 
+		/*
+		 * Preserve the original dlen_type, this way below the EOP logic
+		 * can use dlen_type.
+		 */
 		tmp_dlen = dlen_type & NFDK_DESC_TX_DMA_LEN_HEAD;
 		dma_len -= tmp_dlen;
 		dma_addr += tmp_dlen + 1;
 
+		/*
+		 * The rest of the data (if any) will be in larger dma descritors
+		 * and is handled with the dma_len loop.
+		 */
 		while (pkt) {
 			if (*lmbuf)
 				rte_pktmbuf_free_seg(*lmbuf);
-- 
2.29.3

Re: [PATCH] net/nfp: fix issue of data len exceeds descriptor limitation

[Ferruh Yigit]

On 11/17/2022 8:33 AM, Chaoyong He wrote:
> From: Long Wu <long.wu@corigine.com>
> 
> If dma_len is larger than "NFDK_DESC_TX_DMA_LEN_HEAD", the value of
> dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD maybe less than packet
> head length. Fill maximum dma_len in first tx descriptor to make
> sure the whole head is included in the first descriptor. 

I understand the problem, but impact is not clear. I assume this cause
failure on Tx of the package or Tx a corrupted package maybe but can you
please explain in the commit log what observed problem is fixed?

> In addition,
> add some explanation for NFDK code more readable.
> 
> Fixes: c73dced48c8c ("net/nfp: add NFDk Tx")
> Cc: jin.liu@corigine.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>  drivers/net/nfp/nfp_rxtx.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
> index b8c874d315..ed88d740fa 100644
> --- a/drivers/net/nfp/nfp_rxtx.c
> +++ b/drivers/net/nfp/nfp_rxtx.c
> @@ -1064,6 +1064,7 @@ nfp_net_nfdk_tx_maybe_close_block(struct nfp_net_txq *txq, struct rte_mbuf *pkt)
>  	if (unlikely(n_descs > NFDK_TX_DESC_GATHER_MAX))
>  		return -EINVAL;
>  
> +	/* Under count by 1 (don't count meta) for the round down to work out */
>  	n_descs += !!(pkt->ol_flags & RTE_MBUF_F_TX_TCP_SEG);
>  
>  	if (round_down(txq->wr_p, NFDK_TX_DESC_BLOCK_CNT) !=
> @@ -1180,6 +1181,7 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
>  	/* Sending packets */
>  	while ((npkts < nb_pkts) && free_descs) {
>  		uint32_t type, dma_len, dlen_type, tmp_dlen;
> +		uint32_t tmp_hlen;
>  		int nop_descs, used_descs;
>  
>  		pkt = *(tx_pkts + npkts);
> @@ -1218,8 +1220,23 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
>  		} else {
>  			type = NFDK_DESC_TX_TYPE_GATHER;
>  		}
> +
> +		/* Implicitly truncates to chunk in below logic */
>  		dma_len -= 1;
> -		dlen_type = (NFDK_DESC_TX_DMA_LEN_HEAD & dma_len) |
> +
> +		/*
> +		 * We will do our best to pass as much data as we can in descriptor
> +		 * and we need to make sure the first descriptor includes whole
> +		 * head since there is limitation in firmware side. Sometimes the
> +		 * value of dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD will less

"bitwise and" is confusing while reading, because of 'and', can you
please use '&' instead, I think it is easier to understand that way.

> +		 * than packet head len.
> +		 */
> +		if (dma_len > NFDK_DESC_TX_DMA_LEN_HEAD)
> +			tmp_hlen = NFDK_DESC_TX_DMA_LEN_HEAD;
> +		else
> +			tmp_hlen = dma_len;
> +

What is the point of masking if you already have above check?
Why don't you use 'tmp_hlen' directly, instead of
"(NFDK_DESC_TX_DMA_LEN_HEAD & tmp_hlen)" after above check?

> +		dlen_type = (NFDK_DESC_TX_DMA_LEN_HEAD & tmp_hlen) |

Since 'tmp_hlen' is only used one, you may prefer ternary operator to
get rid of temporary variable, but it is up to you based on readability:

dlen_type = (dma_len > NFDK_DESC_TX_DMA_LEN_HEAD ?
NFDK_DESC_TX_DMA_LEN_HEAD : dma_len) |

>  			(NFDK_DESC_TX_TYPE_HEAD & (type << 12));
>  		ktxds->dma_len_type = rte_cpu_to_le_16(dlen_type);
>  		dma_addr = rte_mbuf_data_iova(pkt);
> @@ -1229,10 +1246,18 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
>  		ktxds->dma_addr_lo = rte_cpu_to_le_32(dma_addr & 0xffffffff);
>  		ktxds++;
>  
> +		/*
> +		 * Preserve the original dlen_type, this way below the EOP logic
> +		 * can use dlen_type.
> +		 */
>  		tmp_dlen = dlen_type & NFDK_DESC_TX_DMA_LEN_HEAD;
>  		dma_len -= tmp_dlen;
>  		dma_addr += tmp_dlen + 1;
>  
> +		/*
> +		 * The rest of the data (if any) will be in larger dma descritors
> +		 * and is handled with the dma_len loop.
> +		 */
>  		while (pkt) {
>  			if (*lmbuf)
>  				rte_pktmbuf_free_seg(*lmbuf);

[PATCH v2] net/nfp: fix issue of data len exceeds descriptor limitation

[Chaoyong He]

From: Long Wu <long.wu@corigine.com>

If dma_len is larger than NFDK_DESC_TX_DMA_LEN_HEAD, the value of
dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD maybe less than packet
head length and the packet will be dropped. Fill maximum dma_len in
first tx descriptor to make sure the whole head is included in the
first descriptor. In addition, add comments to better explain the
code flow.

Fixes: c73dced48c8c ("net/nfp: add NFDk Tx")
Cc: jin.liu@corigine.com
Cc: stable@dpdk.org

Signed-off-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---

v2:
* Rewrite the commit message.
* Revise some logic according to the advice.

---
 drivers/net/nfp/nfp_rxtx.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
index 01cffdfde0..990007c03d 100644
--- a/drivers/net/nfp/nfp_rxtx.c
+++ b/drivers/net/nfp/nfp_rxtx.c
@@ -1063,6 +1063,7 @@ nfp_net_nfdk_tx_maybe_close_block(struct nfp_net_txq *txq, struct rte_mbuf *pkt)
 	if (unlikely(n_descs > NFDK_TX_DESC_GATHER_MAX))
 		return -EINVAL;
 
+	/* Under count by 1 (don't count meta) for the round down to work out */
 	n_descs += !!(pkt->ol_flags & RTE_MBUF_F_TX_TCP_SEG);
 
 	if (round_down(txq->wr_p, NFDK_TX_DESC_BLOCK_CNT) !=
@@ -1219,8 +1220,19 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 		} else {
 			type = NFDK_DESC_TX_TYPE_GATHER;
 		}
+
+		/* Implicitly truncates to chunk in below logic */
 		dma_len -= 1;
-		dlen_type = (NFDK_DESC_TX_DMA_LEN_HEAD & dma_len) |
+
+		/*
+		 * We will do our best to pass as much data as we can in descriptor
+		 * and we need to make sure the first descriptor includes whole
+		 * head since there is limitation in firmware side. Sometimes the
+		 * value of dma_len bitwise & NFDK_DESC_TX_DMA_LEN_HEAD will less
+		 * than packet head len.
+		 */
+		dlen_type = (dma_len > NFDK_DESC_TX_DMA_LEN_HEAD ?
+			NFDK_DESC_TX_DMA_LEN_HEAD : dma_len) |
 			(NFDK_DESC_TX_TYPE_HEAD & (type << 12));
 		ktxds->dma_len_type = rte_cpu_to_le_16(dlen_type);
 		dma_addr = rte_mbuf_data_iova(pkt);
@@ -1230,10 +1242,18 @@ nfp_net_nfdk_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pk
 		ktxds->dma_addr_lo = rte_cpu_to_le_32(dma_addr & 0xffffffff);
 		ktxds++;
 
+		/*
+		 * Preserve the original dlen_type, this way below the EOP logic
+		 * can use dlen_type.
+		 */
 		tmp_dlen = dlen_type & NFDK_DESC_TX_DMA_LEN_HEAD;
 		dma_len -= tmp_dlen;
 		dma_addr += tmp_dlen + 1;
 
+		/*
+		 * The rest of the data (if any) will be in larger dma descritors
+		 * and is handled with the dma_len loop.
+		 */
 		while (pkt) {
 			if (*lmbuf)
 				rte_pktmbuf_free_seg(*lmbuf);
-- 
2.29.3

Re: [PATCH v2] net/nfp: fix issue of data len exceeds descriptor limitation

[Ferruh Yigit]

On 11/29/2022 1:21 AM, Chaoyong He wrote:
> From: Long Wu <long.wu@corigine.com>
> 
> If dma_len is larger than NFDK_DESC_TX_DMA_LEN_HEAD, the value of
> dma_len bitwise and NFDK_DESC_TX_DMA_LEN_HEAD maybe less than packet
> head length and the packet will be dropped. Fill maximum dma_len in
> first tx descriptor to make sure the whole head is included in the
> first descriptor. In addition, add comments to better explain the
> code flow.
> 

updated patch title as:
net/nfp: fix Tx packet drop for large data length

> Fixes: c73dced48c8c ("net/nfp: add NFDk Tx")
> Cc: jin.liu@corigine.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>

Applied to dpdk-next-net/main, thanks.


Below changes done while merging.

> +		/*
> +		 * We will do our best to pass as much data as we can in descriptor
> +		 * and we need to make sure the first descriptor includes whole
> +		 * head since there is limitation in firmware side. Sometimes the
> +		 * value of dma_len bitwise & NFDK_DESC_TX_DMA_LEN_HEAD will less
> +		 * than packet head len.
> +		 */

Updated last line as:
" * value of 'dma_len & NFDK_DESC_TX_DMA_LEN_HEAD' will be less "

<...>

>  
> +		/*
> +		 * The rest of the data (if any) will be in larger dma descritors

s/descritors/descriptors/
s/dma/DMA/