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

[Ori Kam <orika@mellanox.com>]

Hi All



> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, November 5, 2019 3:02 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Ori Kam <orika@mellanox.com>;
> John McNamara <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.com>; dev@dpdk.org; jingjing.wu@intel.com;
> stephen@networkplumber.org; Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Subject: Re: [dpdk-dev] [PATCH v7 02/14] ethdev: add support for hairpin queue
> 
> 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]
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Finbox.dpdk
> .org%2Fdev%2F20191030142938.bpi4txlrebqfq7uw%40platinum%2F&amp;data
> =02%7C01%7Corika%40mellanox.com%7Cb065a7e085aa47723cc408d761f06c9
> f%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C6370855575589250
> 92&amp;sdata=6kY30p%2BEr4DMQiMqbBPX%2BJZ7h0eMp0FxnzhnE%2F7U%2
> BeM%3D&amp;reserved=0
> > >>> 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?
> 
> 

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.

Best,
Ori