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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/5/2019 11:49 AM, Andrew Rybchenko wrote:
> On 11/5/19 2:36 PM, Ori Kam wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Sent: Tuesday, November 5, 2019 1:25 PM
>>> To: Ori Kam <orika@mellanox.com>; John McNamara
>>> <john.mcnamara@intel.com>; Marko Kovacevic
>>> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
>>> Andrew Rybchenko <arybchenko@solarflare.com>
>>> Cc: dev@dpdk.org; jingjing.wu@intel.com; stephen@networkplumber.org; Jerin
>>> Jacob <jerin.jacob@caviumnetworks.com>
>>> Subject: Re: [PATCH v7 02/14] ethdev: add support for hairpin queue
>>>
>>> On 10/30/2019 11:53 PM, Ori Kam wrote:
>>>> This commit introduce hairpin queue type.
>>>>
>>>> The hairpin queue in build from Rx queue binded to Tx queue.
>>>> It is used to offload traffic coming from the wire and redirect it back
>>>> to the wire.
>>>>
>>>> There are 3 new functions:
>>>> - rte_eth_dev_hairpin_capability_get
>>>> - rte_eth_rx_hairpin_queue_setup
>>>> - rte_eth_tx_hairpin_queue_setup
>>>>
>>>> In order to use the queue, there is a need to create rte_flow
>>>> with queue / RSS action that targets one or more of the Rx queues.
>>>>
>>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>
>>> <...>
>>>
>>>>  #include <rte_ethdev_core.h>
>>>>
>>>>  /**
>>>> + * @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);
>>>> +
>>>> +/**
>>>
>>> If these functions are internal why there are in 'rte_ethdev.h' ?
>>>
>>>>   *
>>>>   * Retrieve a burst of input packets from a receive queue of an Ethernet
>>>>   * device. The retrieved packets are stored in *rte_mbuf* structures whose
>>>> @@ -4251,6 +4400,11 @@ int rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t
>>> port_id,
>>>>  		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n",
>>> queue_id);
>>>>  		return 0;
>>>>  	}
>>>> +	if (rte_eth_dev_is_rx_hairpin_queue(dev, queue_id)) {
>>>> +		RTE_ETHDEV_LOG(ERR, "Rx burst failed, queue_id=%u is
>>> hairpin queue\n",
>>>> +			       queue_id);
>>>> +		return 0;
>>>> +	}
>>>>  #endif
>>>>  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
>>>>  				     rx_pkts, nb_pkts);
>>>> @@ -4517,6 +4671,11 @@ static inline int
>>> rte_eth_tx_descriptor_status(uint16_t port_id,
>>>>  		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
>>> queue_id);
>>>>  		return 0;
>>>>  	}
>>>> +	if (rte_eth_dev_is_tx_hairpin_queue(dev, queue_id)) {
>>>> +		RTE_ETHDEV_LOG(ERR, "Tx burst failed, queue_id=%u is
>>> hairpin queue\n",
>>>> +			       queue_id);
>>>> +		return 0;
>>>> +	}
>>>>  #endif
>>>
>>> Hi Ori,
>>>
>>> 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?

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

The question above was do we need this API, or instead should remove the check
from rx/tx_burst APIs?

> 
>>> [1]
>>> /usr/bin/ld: rte_event_eth_rx_adapter.o: in function `rxa_eth_rx':
>>> rte_event_eth_rx_adapter.c:(.text+0x1728): undefined reference to
>>> `rte_eth_dev_is_rx_hairpin_queue'
>>> /usr/bin/ld: rte_event_eth_rx_adapter.o: in function `rxa_service_func':
>>> rte_event_eth_rx_adapter.c:(.text+0x22ab): undefined reference to
>>> `rte_eth_dev_is_rx_hairpin_queue'
>>> /usr/bin/ld: rte_event_eth_tx_adapter.o: in function `txa_service_buffer_retry':
>>> rte_event_eth_tx_adapter.c:(.text+0xa43): undefined reference to
>>> `rte_eth_dev_is_tx_hairpin_queue'
>>> /usr/bin/ld: rte_event_eth_tx_adapter.o: in function `txa_service_func':
>>> rte_event_eth_tx_adapter.c:(.text+0xe7d): undefined reference to
>>> `rte_eth_dev_is_tx_hairpin_queue'
>>> /usr/bin/ld: rte_event_eth_tx_adapter.c:(.text+0x1155): undefined reference to
>>> `rte_eth_dev_is_tx_hairpin_queue'
>>> collect2: error: ld returned 1 exit status
>