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

[Ananyev, Konstantin]

> 
> On 10/4/2021 10:20 AM, Ananyev, Konstantin wrote:
> >
> >>>>
> >>>>>  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?
> >>>
> >>
> >> Make them public API. (Basically just remove '__' prefix and @internal comment).
> >>
> >> This way application can use them to run custom callback(s) (not only the
> >> registered ones), not sure if this can be dangerous though.
> >
> > Hmm, as I said above, I don't want users to call them explicitly.
> > Do you have any good reason to allow it?
> >
> 
> Just to get rid of this internal APIs that is exposed to application state.
> 
> >>
> >> We need to trace the ABI for these functions, making them public clarifies it.
> >
> > We do have plenty of semi-internal functions right now,
> > why adding that one will be a problem?
> 
> As far as I remember existing ones are 'static inline' functions, and we don't
> have an ABI concern with them. But these are actual functions called by application.

Not always.
As an example of internal but not static ones:
rte_mempool_check_cookies
rte_mempool_contig_blocks_check_cookies
rte_mempool_op_calc_mem_size_helper
_rte_pktmbuf_read

> 
> > From other side - if we'll declare it public, we will have obligations to support it
> > in future releases, plus it might encourage users to use it on its own.
> > To me that sounds like extra headache without any gain in return.
> >
> 
> If having those two as public API doesn't make sense, I agree with you.
> 
> >> Also comment can be updated to describe intended usage instead of marking them
> >> internal, and applications can use these anyway if we mark them internal or not.
> >