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