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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-10-27 16:24, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-10-27 15:52, Ananyev, Konstantin:
> > > > Hi Tomasz,
> > > >
> > > > This is a major new function in the API and I still have some comments.
> > > >
> > > > 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?

Please think how we can use it in every applications.
It is not ready.
Either we introduce the API without enabling it, or we implement it
in every drivers.

> > > > >  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.

Yes it could be a separate patch in the same patchset.