Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path

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

On Wed, Apr 7, 2021 at 6:40 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 3/31/2021 9:53 AM, Leyi Rong wrote:
> > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and simple
> > data path selection, as the sanity check is needed ideally.
> >
> > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > ---
> >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > index 61cb204be2..b3d7765e3b 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> >                       PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> >                       dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> >               }
> > -             dev->tx_pkt_prepare = NULL;
> > +             dev->tx_pkt_prepare = i40e_prep_pkts;
> >       } else {
> >               PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> >               dev->tx_pkt_burst = i40e_xmit_pkts;
> >
>
> It seems prepare function is doing some sanity checks before handing packets to
> the HW.
> So with this change all Tx paths calls the same Tx prepare function, if so why
> not set the function pointer outside of the if block, instead of setting it in
> both legs of the if/else? This clarifies that Tx prepare used always.

As I said earlier, what is the point of setting it in ethdev.c too?
It would be more consistent to have this in a single location.

drivers/net/i40e/i40e_ethdev.c: dev->tx_pkt_burst = i40e_xmit_pkts;
drivers/net/i40e/i40e_ethdev.c: dev->tx_pkt_prepare = i40e_prep_pkts;
drivers/net/i40e/i40e_ethdev_vf.c:      eth_dev->tx_pkt_burst = &i40e_xmit_pkts;
drivers/net/i40e/i40e_rxtx.c:   if (dev->tx_pkt_burst ==
i40e_xmit_pkts_vec_avx512) {
drivers/net/i40e/i40e_rxtx.c:   if (dev->tx_pkt_burst ==
i40e_xmit_pkts_vec_avx2 ||
drivers/net/i40e/i40e_rxtx.c:                   dev->tx_pkt_burst ==
i40e_xmit_pkts_vec) {
drivers/net/i40e/i40e_rxtx.c:
dev->tx_pkt_burst = i40e_xmit_pkts_vec_avx512;
drivers/net/i40e/i40e_rxtx.c:
dev->tx_pkt_burst = use_avx2 ?
drivers/net/i40e/i40e_rxtx.c:                   dev->tx_pkt_burst =
i40e_xmit_pkts_simple;
drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_prepare = NULL;
drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_burst = i40e_xmit_pkts;
drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_prepare = i40e_prep_pkts;
drivers/net/i40e/i40e_rxtx.c:   eth_tx_burst_t pkt_burst = dev->tx_pkt_burst;
drivers/net/i40e/i40e_vf_representor.c: ethdev->tx_pkt_burst =
i40e_vf_representor_tx_burst;


-- 
David Marchand