From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Chaoyong He <chaoyong.he@corigine.com>, dev@dpdk.org
Cc: oss-drivers@corigine.com, niklas.soderlund@corigine.com,
Long Wu <long.wu@corigine.com>,
jin.liu@corigine.com, stable@dpdk.org
Subject: Re: [PATCH] net/nfp: fix issue of data len exceeds descriptor limitation
Date: Fri, 18 Nov 2022 11:14:00 +0000 [thread overview]
Message-ID: <b3d681ea-05f4-894a-1db2-65e71eddb96a@amd.com> (raw)
In-Reply-To: <20221117083320.21815-1-chaoyong.he@corigine.com>
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);
next prev parent reply other threads:[~2022-11-18 11:14 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-17 8:33 Chaoyong He
2022-11-18 11:14 ` Ferruh Yigit [this message]
2022-11-29 1:21 ` [PATCH v2] " Chaoyong He
2022-12-07 12:28 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b3d681ea-05f4-894a-1db2-65e71eddb96a@amd.com \
--to=ferruh.yigit@amd.com \
--cc=chaoyong.he@corigine.com \
--cc=dev@dpdk.org \
--cc=jin.liu@corigine.com \
--cc=long.wu@corigine.com \
--cc=niklas.soderlund@corigine.com \
--cc=oss-drivers@corigine.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).