Re: [PATCH v2] app/testpmd: use Tx preparation in txonly engine

[David Marchand <david.marchand@redhat.com>]

On Thu, Jan 11, 2024 at 7:06 AM Kaiwen Deng <kaiwenx.deng@intel.com> wrote:
>
> Txonly forwarding engine does not call the Tx preparation API
> before transmitting packets. This may cause some problems.
>
> TSO breaks when MSS spans more than 8 data fragments. Those
> packets will be dropped by Tx preparation API, but it will cause
> MDD event if txonly forwarding engine does not call the Tx preparation
> API before transmitting packets.
>
> We can reproduce this issue by these steps list blow on ICE and I40e.
>
> ./x86_64-native-linuxapp-gcc/app/dpdk-testpmd -c 0xf -n 4 -- -i
> --tx-offloads=0x00008000
>
> testpmd>set txpkts 64,128,256,512,64,128,256,512,512
> testpmd>set burst 1
> testpmd>start tx_first 1
>
> This commit will use Tx preparation API in txonly forwarding engine.
>
> Fixes: 655131ccf727 ("app/testpmd: factorize fwd engines Tx")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kaiwen Deng <kaiwenx.deng@intel.com>

I did not see a reply to Jerin concern on performance impact of this change.

Some additional comment below.

> ---
>  app/test-pmd/txonly.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
> index c2b88764be..9dc53553a7 100644
> --- a/app/test-pmd/txonly.c
> +++ b/app/test-pmd/txonly.c
> @@ -335,13 +335,16 @@ pkt_burst_transmit(struct fwd_stream *fs)
>         struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
>         struct rte_port *txp;
>         struct rte_mbuf *pkt;
> +       struct rte_mbuf *mb;
>         struct rte_mempool *mbp;
>         struct rte_ether_hdr eth_hdr;
>         uint16_t nb_tx;
>         uint16_t nb_pkt;
> +       uint16_t nb_prep;
>         uint16_t vlan_tci, vlan_tci_outer;
>         uint64_t ol_flags = 0;
>         uint64_t tx_offloads;
> +       char buf[256];
>
>         mbp = current_fwd_lcore()->mbp;
>         txp = &ports[fs->tx_port];
> @@ -396,7 +399,19 @@ pkt_burst_transmit(struct fwd_stream *fs)
>         if (nb_pkt == 0)
>                 return false;
>
> -       nb_tx = common_fwd_stream_transmit(fs, pkts_burst, nb_pkt);
> +       nb_prep = rte_eth_tx_prepare(fs->tx_port, fs->tx_queue,
> +               pkts_burst, nb_pkt);
> +       if (unlikely(nb_prep != nb_pkt)) {

The buf variable declaration can be moved at the start of this block
as buf is not needed anywhere else.

> +               mb = pkts_burst[nb_prep];
> +               rte_get_tx_ol_flag_list(mb->ol_flags, buf, sizeof(buf));

You don't need a mb variable, simply pass pkts_burst[nb_prep]->ol_flags.

rte_get_tx_ol_flag_list() return value must be checked for the
(theoretical) case when the 'buf' variable is too short to avoid the
below fprintf outputting garbage since buf may be unterminated.


> +               fprintf(stderr,
> +                       "Preparing packet burst to transmit failed: %s ol_flags: %s\n",
> +                       rte_strerror(rte_errno), buf);
> +               fs->fwd_dropped += nb_pkt - nb_prep;
> +               rte_pktmbuf_free_bulk(&pkts_burst[nb_prep], nb_pkt - nb_prep);
> +       }
> +
> +       nb_tx = common_fwd_stream_transmit(fs, pkts_burst, nb_prep);
>
>         if (txonly_multi_flow)
>                 RTE_PER_LCORE(_src_port_var) -= nb_pkt - nb_tx;
> --
> 2.34.1
>


-- 
David Marchand