Re: [dpdk-dev] [PATCH v7 02/14] ethdev: add support for hairpin queue

[Thomas Monjalon <thomas@monjalon.net>]

05/11/2019 13:27, Andrew Rybchenko:
> On 11/5/19 3:23 PM, Ferruh Yigit wrote:
> > On 11/5/2019 12:12 PM, Andrew Rybchenko wrote:
> >> On 11/5/19 3:05 PM, Ferruh Yigit wrote:
> >>> On 11/5/2019 11:49 AM, Andrew Rybchenko wrote:
> >>>> On 11/5/19 2:36 PM, Ori Kam wrote:
 >>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>>>>  /**
> >>>>>>> + * @internal
> >>>>>>> + * Check if the selected Rx queue is hairpin queue.
> >>>>>>> + *
> >>>>>>> + * @param dev
> >>>>>>> + *  Pointer to the selected device.
> >>>>>>> + * @param queue_id
> >>>>>>> + *  The selected queue.
> >>>>>>> + *
> >>>>>>> + * @return
> >>>>>>> + *   - (1) if the queue is hairpin queue, 0 otherwise.
> >>>>>>> + */
> >>>>>>> +int
> >>>>>>> +rte_eth_dev_is_rx_hairpin_queue(struct rte_eth_dev *dev, uint16_t
> >>>>>> queue_id);
> >>>>>>> +
> >>>>>>> +/**
> >>>>>>> + * @internal
> >>>>>>> + * Check if the selected Tx queue is hairpin queue.
> >>>>>>> + *
> >>>>>>> + * @param dev
> >>>>>>> + *  Pointer to the selected device.
> >>>>>>> + * @param queue_id
> >>>>>>> + *  The selected queue.
> >>>>>>> + *
> >>>>>>> + * @return
> >>>>>>> + *   - (1) if the queue is hairpin queue, 0 otherwise.
> >>>>>>> + */
> >>>>>>> +int
> >>>>>>> +rte_eth_dev_is_tx_hairpin_queue(struct rte_eth_dev *dev, uint16_t
> >>>>>> queue_id);
[...]
> >>>>>> These are causing build error, thanks Jerin for catching, because they are
> >>>>>> internal and called by a public static inline API, so whoever calls
> >>>>>> 'rte_eth_rx/tx_burst()' APIs in the shared build, can't find
> >>>>>> 'rte_eth_dev_is_rx/tx_hairpin_queue()' functions [1],
> >>>>>>
> >>>>>> as far as I can see there are two options:
> >>>>>> 1) Remove these checks
> >>>>>> 2) Make 'rte_eth_dev_is_rx/tx_hairpin_queue()' public API instead of internal
> >>>>>>
> >>>>>> If there is a value to make 'rte_eth_dev_is_rx/tx_hairpin_queue()' public API
> >>>>>> we
> >>>>>> should go with (2) else (1).
> >>>>>>
> >>>>> I think we can skip the tests,
> >>>>> But it was Andrew request so we must get is response.
> >>>>> It was also his empathies that they should be internal.
> >>>> It is important for me to keep rte_eth_dev_state internal and
> >>>> few patches ago rte_eth_dev_is_rx_hairpin_queue() was inline.
> >>> Are you saying you don't want to option to make
> >>> 'rte_eth_dev_is_rx_hairpin_queue()' static inline because it will force the
> >>> 'RTE_ETH_QUEUE_STATE_xxx' being public?
> >> Yes.
> > +1
> >
> >>>> I'm OK to make the function experimental or keep it internal
> >>>> (no API/ABI stability requirements) but externally visible (in .map).
> >>> I think we can't do this, add a function deceleration to the public header file
> >>> and add it to the .map file but keep it internal. Instead we can make it a
> >>> proper API and it should be experimental at least first release.
> >> We have discussed similar thing with Olivier recently [1].
> >>
> >> [1] http://inbox.dpdk.org/dev/20191030142938.bpi4txlrebqfq7uw@platinum/
> > Yes we can say they are internal but there won't be anything preventing
> > applications to use them.
> 
> That's true, but making it internal says - don't use it.
> Anyway, I have no strong opinion on experimental vs internal.
> 
> >>> The question above was do we need this API, or instead should remove the check
> >>> from rx/tx_burst APIs?
> >> I think these checks are useful to ensure that these functions
> >> are not used for hairpin queues. At least to catch it with debug
> >> enabled.
> > OK, if so what not make them proper API? Any concern on it?

Why we should not use this API in applications?