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

[Thomas Monjalon <thomas@monjalon.net>]

05/11/2019 14:23, Ori Kam:
> From: 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.

Using an experimental function in an inline API may propagate the
experimental flag further.

> Moving the API to experimental results in the following error:
> error: 'rte_eth_dev_is_rx_hairpin_queue' is deprecated (declared at /.autodirect/mtrswgwork/orika/pegasus04_share/dpdk.org/x86_64-native-linuxapp-gcc/include/rte_ethdev.h:4276): Symbol is not yet part of stable ABI [-Werror=deprecated-declarations]
> 
> I suggest that we remove the checks, in any case this checks are only on debug mode, and when using data path the user must be very careful and what he
> is doing.

I agree it seems better to remove these checks for now.
We can decide to re-introduce them later as non-experimental.