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

["Rong, Leyi" <leyi.rong@intel.com>]

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, April 8, 2021 6:33 PM
> To: Rong, Leyi <leyi.rong@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data
> path
> 
> 
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > > Sent: Thursday, April 8, 2021 12:40 AM
> > > To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for
> > > vector data path
> > >
> > > 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.
> >
> > Hi Ferruh,
> >
> > Yes, it make sense.
> >
> > Hi Konstantin,
> 
> Hi Leyi,
> 
> >
> > Would that be something wrong if the prepare function goes for simple Tx
> function although it does not support the offload feature yet?
> >
> 
> Current situation:
> For simple TX path we set dev->tx_pkt_prepare = NULL.
> That makes rte_eth_tx_prepare() a stub that does nothing and always returns:
> "All packets are good".
> That is unsafe off-course, and if upper layer will pass a packet that is not
> supported, then it can lead to various bad things: bad cksum, corrupted packets,
> TX hang, etc.
> But at least it keeps simple TX path fast.
> With that patch:
> For simple TX path we set dev->tx_pkt_prepare = i40e_prep_pkts.
> Now on TX path we invoke extra function that does a lot of checks, but it still
> unsafe:
> as i40e_prep_pkts() assumes that  full-featured TX function is in place (multi-segs
> are allowed, etc.).
> So our simple TX path became slower, but still is unsafe.
> I think that if we want to introduce tx_prepare() for simple TX path, then the
> proper way - create a new function for it (i40e_simple_prep_pkts() or so).
> It will be aware that simple TX path is in place and more restrictions should be
> met:
> check that nb_segs==1 and no TX offloads (except FAST_FREE?) are enabled,
> plus usual checks for min and max pkt_len.
> 
> Konstantin
> 

Hi Konstantin,

Thanks for the explanation, I know the current full-featured prepare function will cost more CPU cycle, but not sure how to say is still unsafe?
Why I set the simple Tx prepare function to the current i40e_prep_pkts() is we may support more offload features like current full-featured Tx for vector path(which is included in simple Tx currently), if so, the current tx prepare function can be re-used.