On Fri, Oct 28, 2016 at 10:15:47AM +0000, Ananyev, Konstantin wrote: > Hi Tomasz, > > > > > > 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
Just my 2 cents, As "tx_prep" is a generic API and if PMD tries to fix-up some other limitation(not csum) then in that case it is difficult for the application to know in which PMD&application combination it needs be used. > or so? > Looking at current testpmd patch, I suppose the changes will be minimal. > What do you think? > Konstantin > > > > > Tomasz > > > > > > > > > > > > > struct rte_eth_dev { > > > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive > > > function. */ > > > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit > > > > > > > function. */ > > > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit > > > > > > > +prepare function. */ > > > > > > > struct rte_eth_dev_data *data; /**< Pointer to device data */ > > > > > > > const struct eth_driver *driver;/**< Driver for this device */ > > > > > > > const struct eth_dev_ops *dev_ops; /**< Functions exported by > > > > > > > PMD */ > > > > > > > > > > > > Could you confirm why tx_pkt_prep is not in dev_ops? > > > > > > I guess we want to have several implementations? > > > > > > > > > > Yes, it depends on configuration options, same as tx_pkt_burst. > > > > > > > > > > > > > > > > > Shouldn't we have a const struct control_dev_ops and a struct > > > datapath_dev_ops? > > > > > > > > > > That's probably a good idea, but I suppose it is out of scope for that > > > patch. > > > > > > > > No it's not out of scope. > > > > It answers to the question "why is it added in this structure and not > > > dev_ops". > > > > We won't do this change when nothing else is changed in the struct. > > > > > > Not sure I understood you here: > > > Are you saying datapath_dev_ops/controlpath_dev_ops have to be introduced > > > as part of that patch? > > > But that's a lot of changes all over rte_ethdev.[h,c]. > > > It definitely worse a separate patch (might be some discussion) for me. > > > Konstantin > > > > > > >