[dpdk-dev] [PATCH v3 4/7] ethdev: make burst functions to use new flat array

[Ananyev, Konstantin]

> On 10/1/2021 3:02 PM, Konstantin Ananyev wrote:
> > Rework 'fast' burst functions to use rte_eth_fp_ops[].
> > While it is an API/ABI breakage, this change is intended to be
> > transparent for both users (no changes in user app is required) and
> > PMD developers (no changes in PMD is required).
> > One extra thing to note - RX/TX callback invocation will cause extra
> > function call with these changes. That might cause some insignificant
> > slowdown for code-path where RX/TX callbacks are heavily involved.
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
> 
> <...>
> 
> >  static inline int
> >  rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id)
> >  {
> > -	struct rte_eth_dev *dev;
> > +	struct rte_eth_fp_ops *p;
> > +	void *qd;
> > +
> > +	if (port_id >= RTE_MAX_ETHPORTS ||
> > +			queue_id >= RTE_MAX_QUEUES_PER_PORT) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			"Invalid port_id=%u or queue_id=%u\n",
> > +			port_id, queue_id);
> > +		return -EINVAL;
> > +	}
> 
> Should the checkes wrapped with '#ifdef RTE_ETHDEV_DEBUG_RX' like others?

Original rte_eth_rx_queue_count() always have similar checks enabled,
that's why I also kept them 'always on'. 

> 
> <...>
> 
> > +++ b/lib/ethdev/version.map
> > @@ -247,11 +247,16 @@ EXPERIMENTAL {
> >  	rte_mtr_meter_policy_delete;
> >  	rte_mtr_meter_policy_update;
> >  	rte_mtr_meter_policy_validate;
> > +
> > +	# added in 21.05
> 
> s/21.05/21.11/
> 
> > +	__rte_eth_rx_epilog;
> > +	__rte_eth_tx_prolog;
> 
> These are directly called by application and must be part of ABI, but marked as
> 'internal' and has '__rte' prefix to highligh it, this may be confusing.
> What about making them proper, non-internal, API?

Hmm not sure what do you suggest here.
We don't want users to call them explicitly.
They are sort of helpers for rte_eth_rx_burst/rte_eth_tx_burst.
So I did what I thought is our usual policy for such semi-internal thigns:
have '@intenal' in comments, but in version.map put them under EXPERIMETAL/global
section.

What do you think it should be instead?
 
> >  };
> >
> >  INTERNAL {
> >  	global:
> >
> > +	rte_eth_fp_ops;
> 
> This variable is accessed in inline function, so accessed by application, not
> sure if it suits the 'internal' object definition, internal should be only for
> objects accessed by other parts of DPDK.
> I think this can be added to 'DPDK_22'.
> 
> >  	rte_eth_dev_allocate;
> >  	rte_eth_dev_allocated;
> >  	rte_eth_dev_attach_secondary;
> >