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