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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

On Tue, Sep 20, 2016 at 09:06:42AM +0000, Ananyev, Konstantin wrote:
> 
> Hi Jerin,

Hi Konstantin,

> 
> > > > >
> > >
> > > [...]
> > >
> > > > > +
> > > > > +#ifdef RTE_ETHDEV_TX_PREP
> > > >
> > > > Sorry for being a bit late on that discussion, but what the point of
> > > > having that config macro (RTE_ETHDEV_TX_PREP ) at all?
> > > > As I can see right now, if driver doesn't setup tx_pkt_prep, then
> > > > nb_pkts would be return anyway...
> > > >
> > > > BTW, there is my another question - should it be that way?
> > > > Shouldn't we return 0 (and set rte_errno=ENOTSUP) here if
> > > > dev->tx_pk_prep == NULL?
> > > >
> > >
> > > It's an answer to the Jerin's request discussed here:
> > > http://dpdk.org/ml/archives/dev/2016-September/046437.html
> > >
> > > When driver doesn't support tx_prep, default behavior is "we don't know requirements, so we have nothing to do here". It will simplify
> > application logic and improve performance for these drivers, I think. Catching this error with every burst may be problematic.
> > >
> > > As for RTE_ETHDEV_TX_PREP macro, suggested by Jerin in the same thread, I still don't think It's the best solution of the problem
> > described by him. I have added it here for further discussion.
> > >
> > > Jerin, have you something to add?
> > 
> > Nothing very specific to add here. I think, I have tried to share the rational in, http://dpdk.org/ml/archives/dev/2016-
> > September/046437.html
> > 
> 
> Ok, not sure I am fully understand your intention here.
> I think I understand why you propose rte_eth_tx_prep() to do:
> 	if (!dev->tx_pkt_prep)
> 		return nb_pkts;
> 
> That allows drivers to NOOP the tx_prep functionality without paying the
> price for callback invocation.

In true sense, returning the nb_pkts makes it functional NOP as well(i.e The PMD
does not have any limitation on Tx side, so all packets are _good_(no
preparation is required))


> What I don't understand, why with that in place we also need a NOOP for
> the whole rte_eth_tx_prep():
> +static inline uint16_t
> +rte_eth_tx_prep(uint8_t port_id __rte_unused, uint16_t queue_id __rte_unused,
> +		struct rte_mbuf **tx_pkts __rte_unused, uint16_t nb_pkts)
> +{
> +	return nb_pkts;
> +}
> +
> +#endif
> 
> What are you trying to save here: just reading ' dev->tx_pkt_prep'?
> If so, then it seems not that performance critical for me.
> Something else?

The proposed scheme can make it as true NOOP from compiler perspective too if a
target decided to do that,
I have checked the instruction generation with arm Assembly, a non true
compiler NOOP has following instructions overhead at minimum.

	# 1 load
	# 1  mov
	if (!dev->tx_pkt_prep)
		return nb_pkts;

	# compile can't predict this function needs be executed or not so
	# pressure on register allocation and mostly likely it call for
	# some stack push and pop based load on outer function(as it is an
	# inline function)

	return (*dev->tx_pkt_prep)(dev->data->tx_queues[queue_id], tx_pkts, nb_pkts);

	# 1 branch
	if (unlikely(nb_prep < nb_rx)) {
		# bunch of code not executed, but pressure on i cache
		int i;
		for (i = nb_prep; i < nb_rx; i++)
	                 rte_pktmbuf_free(pkts_burst[i]);
	}

>From a server target(IA or high-end armv8) with external PCIe based system makes sense to have
RTE_ETHDEV_TX_PREP option enabled(which is the case in proposed patch) but
the low end arm platforms with
- limited cores
- less i cache
- IPC == 1
- running around 1GHz
- most importantly, _integrated_ nic controller with no external PCIE
  support
does not make much sense to waste the cycles/time for it.
cycle saved is cycle earned.

Since DPDK compilation is based on _target_, I really don't see any
issue with this approach nor it does not hurt anything on server target.
So, IMO, It should be upto the target to decide what works better for the target.

Jerin

> From my point of view NOOP on the driver level is more than enough.
> Again I would prefer to introduce new config option, if possible.
> 
> Konstantin
> 
> 
> 
> 
> 
> 
>