Re: [dpdk-dev] [PATCH 01/13] ethdev: support setup function for hairpin queue

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi Ori,

@Thomas, @Ferruh, please, see question below.

On 10/2/19 3:19 PM, Ori Kam wrote:
> Hi Andrew,
>
> Sorry it took me some time to responded, (I'm on vacation 😊)
> I think we are in most cases in agreement. The only open issue is the
> checks so please see my comments below.
> As soon as we get to understanding about this issue, I will start working on V2.
>
> Thanks,
> Ori

[snip]

>>>>>>> @@ -1769,6 +1793,60 @@ int rte_eth_rx_queue_setup(uint16_t port_id,
>>>>>> uint16_t rx_queue_id,
>>>>>>>      		struct rte_mempool *mb_pool);
>>>>>>>
>>>>>>>      /**
>>>>>>> + * @warning
>>>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without prior
>>>>>>> + notice
>>>>>>> + *
>>>>>>> + * Allocate and set up a hairpin receive queue for an Ethernet device.
>>>>>>> + *
>>>>>>> + * The function set up the selected queue to be used in hairpin.
>>>>>>> + *
>>>>>>> + * @param port_id
>>>>>>> + *   The port identifier of the Ethernet device.
>>>>>>> + * @param rx_queue_id
>>>>>>> + *   The index of the receive queue to set up.
>>>>>>> + *   The value must be in the range [0, nb_rx_queue - 1] previously supplied
>>>>>>> + *   to rte_eth_dev_configure().
>>>>>> Is any Rx queue may be setup as hairpin queue?
>>>>>> Can it be still used for regular traffic?
>>>>>>
>>>>> No if a queue is used as hairpin it can't be used for normal traffic.
>>>>> This is also why I like the idea of two different functions, in order to create
>>>>> This distinction.
>>>> If so, do we need at least debug-level checks in Tx/Rx burst functions?
>>>> Is it required to patch rte flow RSS action to ensure that Rx queues of
>>>> only one kind are specified?
>>>> What about attempt to add Rx/Tx callbacks for hairpin queues?
>>>>
>>> I think the checks should be done in PMD level. Since from high level they are the
>>> same.
>> Sorry, I don't understand why. If something could be checked on generic level,
>> it should be done to avoid duplication in all drivers.
> The issue with this approach is that at the ethdev level we don't know anything about the queue.
> This will mean that we will need to add extra functions to query the queue type for each PMD.
> We could also assume that if to get type function exist in the pmd then the queue is always standard queue.
> So my suggestion if you would like to move the checks is to add queue type enum in the ethdev level, and add
> function call to query the queue type. What do you think?

I would consider to use dev_data rx_queue_state and tx_queue_state to
keep the information to have it directly available without extra function
calls. Or add extra information. dev_data is internal and it looks like not
a problem. What do you think?

>>> Call backs for Rx/Tx doesn't make sense, since the idea is to bypass the CPU.
>> If so, I think rte_eth_add_tx_callback() should be patched to return an
>> error
>> if specified queue is hairpin. Same for Rx.
>> Any other cases?
>>
> Same answer as above.

[snip]

Andrew.