Re: [dpdk-dev] [PATCH 1/8] net/bnxt: remove assert for zero data len in Tx path

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

On Mon, Jun 20, 2022 at 3:55 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>
> On 6/20/2022 12:09 AM, Ajit Khaparde wrote:
> > On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
> >>
> >> On 6/15/2022 3:56 PM, Kalesh A P wrote:
> >>>
> >>> From: Somnath Kotur <somnath.kotur@broadcom.com>
> >>>
> >>> Currently the PMD tries to detect a potential 0 byte DMA by
> >>> using RTE_VERIFY.
> >>> But since RTE_VERIFY internally calls rte_panic() it is fatal to
> >>> the application and some applications want to avoid that.
> >>> So return an error from the bnxt xmit handler if such a bad pkt is
> >>> encountered by logging an error message, dumping the pkt header and
> >>> dump the current stack as well
> >>>
> >>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> >>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> >>> ---
> >>>    drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
> >>>    1 file changed, 26 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> >>> index 7a7196a..67e0167 100644
> >>> --- a/drivers/net/bnxt/bnxt_txr.c
> >>> +++ b/drivers/net/bnxt/bnxt_txr.c
> >>> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
> >>>           return false;
> >>>    }
> >>>
> >>> +static bool
> >>> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
> >>> +{
> >>> +       const char *type_str = "Data len";
> >>> +       uint16_t len_to_check = tx_pkt->data_len;
> >>> +
> >>> +       if (data_len_chk == 0) {
> >>> +               type_str = "TSO Seg size";
> >>> +               len_to_check = tx_pkt->tso_segsz;
> >>> +       }
> >>> +
> >>> +       if (len_to_check == 0) {
> >>> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
> >>> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
> >>> +               rte_dump_stack();
> >>> +               return true;
> >>> +       }
> >>> +       return false;
> >>> +}
> >>> +
> >>>    static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >>>                                   struct bnxt_tx_queue *txq,
> >>>                                   uint16_t *coal_pkts,
> >>> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
> >>>           }
> >>>
> >>>           /* Check non zero data_len */
> >>> -       RTE_VERIFY(tx_pkt->data_len);
> >>> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
> >>> +               return -EIO;
> >>>
> >>
> >> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
> >> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
> >> data path code, do you want to do the same?
> >
> >
> > When we originally added these checks, we were not sure how prevalent
> > is the usage of tx_pkt_prepare() dev_op by various applications.
> >
> > We will stick with this patch for now  and implement that
> > rte_eth_tx_prepare() in the next release?
> >
>
> 'rte_eth_tx_prepare()' is not mandatory, so yes there may be
> applications that are not calling this API.
>
> If we have a consensus to have checks in the prepare function, I think
> it helps both to PMDs and applications.
I agree.
Once the changes for tx_pkt_prepare() are ready,
we will remove the checks from the Tx burst handler.
BNXT PMD will carry the current checks till then.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]