On 11/5/19 4:02 PM, Thomas Monjalon wrote: > 05/11/2019 13:53, Andrew Rybchenko: >> On 11/5/19 3:51 PM, Thomas Monjalon wrote: >>> 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.yi...@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? >> I think the valid question is why application needs the API. >> Basically I don't mind, just want to be sure that only required >> API is exposed. > Because hairpin queues are not standard queues, > we may need to distinguish them. > I see it as a good helper for applications. > Am I missing something obvious?
I think no. I would prefer explicit reason, but since these function are simple, I'm OK to go with "may need to distinguish them"
