Re: [PATCH v11 1/6] ethdev: add Tx preparation

["Richardson, Bruce" <bruce.richardson@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Friday, October 28, 2016 11:29 AM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Kulasek, TomaszX
> <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, October 28, 2016 11:22 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Kulasek,
> > TomaszX <tomaszx.kulasek@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> >
> > 2016-10-28 10:15, Ananyev, Konstantin:
> > > > From: Ananyev, Konstantin
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > > > > > 2016-10-26 14:56, Tomasz Kulasek:
> > > > > > > > > --- a/config/common_base
> > > > > > > > > +++ b/config/common_base
> > > > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=y
> > > > > > > >
> > > > > > > > We cannot enable it until it is implemented in every
> drivers.
> > > > > > >
> > > > > > > Not sure why?
> > > > > > > If tx_pkt_prep == NULL, then rte_eth_tx_prep() would just act
> as noop.
> > > > > > > Right now it is not mandatory for the PMD to implement it.
> > > > > >
> > > > > > If it is not implemented, the application must do the
> > > > > > preparation by
> > > > > itself.
> > > > > > From patch 6:
> > > > > > "
> > > > > > Removed pseudo header calculation for udp/tcp/tso packets from
> > > > > > application and used Tx preparation API for packet preparation
> > > > > > and verification.
> > > > > > "
> > > > > > So how does it behave with other drivers?
> > > > >
> > > > > Hmm so it seems that we broke testpmd csumonly mode for
> > > > > non-intel drivers..
> > > > > My bad, missed that part completely.
> > > > > Yes, then I suppose for now we'll need to support both (with and
> > > > > without) code paths for testpmd.
> > > > > Probably a new fwd mode or just extra parameter for the existing
> one?
> > > > > Any other suggestions?
> > > > >
> > > >
> > > > I had sent txprep engine in v2
> > > > (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on
> > > > the suggestions. If you like it I can
> > resent
> > > > it in place of csumonly modification.
> > >
> > > I still not sure it is worth to have another version of csum...
> > > Can we introduce a new global variable in testpmd and a new command:
> > > testpmd> csum tx_prep
> > > or so?
> > > Looking at current testpmd patch, I suppose the changes will be
> minimal.
> > > What do you think?
> >
> > No please no!
> > The problem is not in testpmd.
> > The problem is in every applications.
> > Should we prepare the checksums or let tx_prep do it?
> 
> Not sure, I understood you...
> Right now we don't' change other apps.
> They would work as before.
> If people would like to start to use tx_prep in their apps - they are free
> to do that.
> If they like to keep doing that manually - that's fine too.
> From other side we need an ability to test (and demonstrate) that new
> functionality.
> So we do need changes in testpmd.
> Konstantin
> 

Just my 2c on this:
* given this is new functionality, and no apps are currently using it, I'm not sure I see the harm in having the function available by default. We just need to be clear about the limits of the function and the fact that apps need to do work themselves if the driver doesn't provide the function.
* having it enabled will then allow any apps that want to use it do to so.
* however, for our sample apps, and by default in testpmd, we *shouldn't* use this functionality, in the absence of any fallback, so that is where I would look to have the enable/disable switch, not in the library.
* going forward, I think a SW fallback inside the ethdev API itself would be a good addition to make this fully generic.

Hope this helps, [and also that I haven't missed some subtlety in the discussion!]

/Bruce