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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> >
> > > > > > > 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 Leyi,
> >
> > > 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?
> >
> > Let say user will do:
> >
> > mb = create_and_fill_multi_seg_pkt(...);
> > n =  rte_eth_tx_prepare(p, q, &mb, 1);
> > if (n == 1)
> >   n = rte_eth_tx_burst(p, q, &mb, 1);
> > else
> >   rte_pktmbuf_free(mb);
> >
> > if dev->tx_pkt_prepare == i40e_prep_pkts and dev->tx_pkt_burs ==
> > i40e_xmit_pkts_simple, then this code will TX the packet, even though it
> > shouldn't in theory.
> >
> 
> Hi Konstantin,
> 
> Yes, it make sense for the current situation.

Hmm, could you elaborate a bit: what exactly makes sense?

> 
> > > 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.
> >
> > AFAIK, for i40e current simple (and vector) TX path doesn't support all offloads
> > that are supported by full-featured path To be more specific: mulit-seg packets,
> > TCP_CKSUM, TCP_SEG, etc.
> > Am I missing something obvious here?
> >
> > Konstantin
> 
> We're intending to support more offload features into vector path gradually, ice/iavf PMD will support Tx checksum offload in AVX512 path
> in the 2105 release, also will try to support more in the future if possible.

Well, when i40e vector TX will support these offloads, then it will make sense to
use the same prepare function for it. 
But right now, I think it is not the case.
So I still think create a separate one is the best approach for now.