Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation

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

2016-12-01 22:31, Kulasek, TomaszX:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-12-01 19:20, Kulasek, TomaszX:
> > > Hi Thomas,
> > >
> > > Sorry, I have answered for this question in another thread and I missed
> > about this one. Detailed answer is below.
> > 
> > Yes you already gave this answer.
> > And I will continue asking the question until you understand it.
> > 
> > > > 2016-11-28 11:54, Thomas Monjalon:
> > > > > Hi,
> > > > >
> > > > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > > > --- a/config/common_base
> > > > > > +++ b/config/common_base
> > > > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > > > > >  CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > >  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > >  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > > >
> > > > > Please, remind me why is there a configuration here.
> > > > > It should be the responsibility of the application to call
> > > > > tx_prepare or not. If the application choose to use this new API
> > > > > but it is disabled, then the packets won't be prepared and there is
> > no error code:
> > > > >
> > > > > > +#else
> > > > > > +
> > > > > > +static inline uint16_t
> > > > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused
> > > > uint16_t queue_id,
> > > > > > +               __rte_unused struct rte_mbuf **tx_pkts, uint16_t
> > > > > > +nb_pkts) {
> > > > > > +       return nb_pkts;
> > > > > > +}
> > > > > > +
> > > > > > +#endif
> > > > >
> > > > > So the application is not aware of the issue and it will not use
> > > > > any fallback.
> > >
> > > tx_prepare mechanism can be turned off by compilation flag (as discussed
> > with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real
> > NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory
> > dereference and check can have significant impact on performance).
> > >
> > > Jerin observed that on some architectures (e.g. low-end ARM with
> > embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause
> > significant performance drop, so he proposed to introduce this
> > configuration flag to provide real NOOP when tx_prepare functionality is
> > not required, and can be turned on based on the _target_ configuration.
> > >
> > > For other cases, when this flag is turned on (by default), and
> > tx_prepare is not implemented, functional NOOP is used based on comparison
> > (dev->tx_pkt_prepare == NULL).
> > 
> > So if the application call this function and if it is disabled, it simply
> > won't work. Packets won't be prepared, checksum won't be computed.
> > 
> > I give up, I just NACK.
> 
> It is not to be turned on/off whatever someone wants, but only and only for the case, when platform developer knows, that his platform doesn't need this callback, so, he may turn off it and then save some performance (this option is per target).

How may he know? There is no comment in the config file, no documentation.

> For this case, the behavior of tx_prepare will be exactly the same when it is turned on or off. If is not the same, there's no sense to turn it off. There were long topic, where we've tried to convince you, that it should be turned on for all devices.

Really? You tried to convince me to turn it on?
No you were trying to convince Jerin.
I think it is a wrong idea to allow disabling this function.
I didn't comment in first discussion because Jerin told it was really
important for small hardware with fixed NIC, and I thought it would
be implemented in a way the application cannot be misleaded.

The only solution I see here is to add some comments in the configuration
file, below the #else and in the doc.
Have you checked doc/guides/prog_guide/poll_mode_drv.rst?