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:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ferruh Yigit <ferruh.yi...@intel.com>
>>>>>> Sent: Tuesday, November 5, 2019 1:25 PM
>>>>>> To: Ori Kam <or...@mellanox.com>; John McNamara
>>>>>> <john.mcnam...@intel.com>; Marko Kovacevic
>>>>>> <marko.kovace...@intel.com>; Thomas Monjalon <tho...@monjalon.net>;
>>>>>> Andrew Rybchenko <arybche...@solarflare.com>
>>>>>> Cc: dev@dpdk.org; jingjing...@intel.com; step...@networkplumber.org;
>>>>>> Jerin
>>>>>> Jacob <jerin.ja...@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 <or...@mellanox.com>
>>>>>>> Reviewed-by: Andrew Rybchenko <arybche...@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?
>> 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?
>
>>>>>> [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
