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

[Thomas Monjalon <thomas@monjalon.net>]

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