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

[Ferruh Yigit]

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.

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