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

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi Ori,

On 9/28/19 6:19 PM, Ori Kam wrote:
> Hi Andrew.
> PSB
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Thursday, September 26, 2019 8:24 PM
>> To: Ori Kam <orika@mellanox.com>; Thomas Monjalon
>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; jingjing.wu@intel.com; stephen@networkplumber.org
>> Subject: Re: [dpdk-dev] [PATCH 01/13] ethdev: support setup function for
>> hairpin queue
>>
>> On 9/26/19 6:58 PM, Ori Kam wrote:
>>> Hi Andrew,
>>> Thanks for your comments PSB.
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> On 9/26/19 9:28 AM, Ori Kam wrote:
>>>>> This commit introduce the RX/TX hairpin setup function.
>>>>> RX/TX should be Rx/Tx here and everywhere below.
>>>>>
>>>>> Hairpin is RX/TX queue that is used by the nic in order to offload
>>>>> wire to wire traffic.
>>>>>
>>>>> Each hairpin queue is binded to one or more queues from other type.
>>>>> For example TX hairpin queue should be binded to at least 1 RX hairpin
>>>>> queue and vice versa.
>>>> How should application find out that hairpin queues are supported?
>>> It should be stated in the release note of the DPDK, when manufacture adds
>> support for this.
>>> In addition if the application try to set hairpin queue and it fails it can mean
>> depending on the
>>> error that the hairpin is not supported.
>> I'm talking about dev_info-like information. Documentation is nice, but
>> it is not
>> very useful to implement application which works with NICs from
>> different vendors.
>>
> What if we add get hairpin capabilities function.
> We could have,  the max number of queues, if the nic support 1:n connection,
> which offloads are supported and so on. So basically create a new set of capabilities
> for hairpin this I think will also remove other concern that you have.
> What do you think?

Yes, I think an API to report capabilities would be useful.
It should be also used in setup functions in order to do checks on
generic level that setup request is OK vs caps.

>>>> How many?
>>> There is no limit to the number of hairpin queues from application all queues
>> can be hairpin queues.
>>
>> I'm pretty sure that it could be vendor specific.
>>
> Please see my answer above.
>   
>>>> How should application find out which ports/queues could be used for
>>>> pining?
>>> All ports and queues can be supported, if the application request invalid
>> combination, for example
>>> in current Mellanox implementation binding between two ports then the
>> setup function will  fail.
>>> If you would like I can add capability for this, but there are too many options.
>> For example number
>>> of queues, binding limitations all of those will be very hard to declare.
>>>
>>>
>>>> Is hair-pinning domain on device level sufficient to expose limitations?
>>>>
>>> I'm sorry but I don’t understand your question.
>> I was just trying to imagine how we could  say that we can hairpin
>> one port Rx queues to another port Tx queues.
>>
> Like I suggested above if I will add a capability function we could have
> a field that says port_binidng supported, or something else, along this line.

Not sure that I understand, but I'll take a look when submitted.

>>>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>>>> ---
>>>>>     lib/librte_ethdev/rte_ethdev.c           | 213
>>>>> +++++++++++++++++++++++++++++++
>>>>>     lib/librte_ethdev/rte_ethdev.h           | 145 +++++++++++++++++++++
>>>>>     lib/librte_ethdev/rte_ethdev_core.h      |  18 +++
>>>>>     lib/librte_ethdev/rte_ethdev_version.map |   4 +
>>>>>     4 files changed, 380 insertions(+)
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>>>> b/lib/librte_ethdev/rte_ethdev.c index 30b0c78..4021f38 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>>> @@ -1701,6 +1701,115 @@ struct rte_eth_dev *
>>>>>     }
>>>>>
>>>>>     int
>>>>> +rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t
>>>>> rx_queue_id,
>>>>> +			       uint16_t nb_rx_desc, unsigned int socket_id,
>>>>> +			       const struct rte_eth_rxconf *rx_conf,
>>>>> +			       const struct rte_eth_hairpin_conf *hairpin_conf)
>>>>> Below code duplicates rte_eth_rx_queue_setup() a lot and it is very bad
>>>>> from maintenance point of view. Similar problem with Tx hairpin queue
>>>>> setup.
>>>>>
>>> I'm aware of that. The reasons I choose it are: (same goes to Tx)
>>> 1. use the same function approach, meaning to use the current  setup
>> function
>>>       the issues with this are:
>>>        * API break.
>>>        * It will have extra parameters, for example mempool will not be used
>>>           for hairpin and hairpin configuration will not be used for normal queue.
>>>           It is possible to use a struct but again API break and some fields are not
>> used.
>>>        * we are just starting with the hairpin, most likely there will be
>> modification so
>>>            it is better to have a different function.
>>>        * From application he undertand that this is a different kind of queue,
>> which shouldn't be
>>>            used by the application.
>> It does not excuse to duplicate so much code below. If we have separate
>> dev_info-like limitations for hairpin, it would make sense, but I hope that
>> it would be still possible to avoid code duplication.
>>
> We can start with the most basic implementation, which will mean that the function
> will almost be empty, when other vendors or Mellanox will require some additional
> test or code they will be able to decide if to add new code to he function, or
> extract the shared code from the standard function to a specific function, and
> use this function in both setup functions.
> What do you think?

Let's try and take a look at the code.

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

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

>>>>> + * @param nb_rx_desc
>>>>> + *   The number of receive descriptors to allocate for the receive ring.
>>>> Does it still make sense for hairpin queue?
>>>>
>>> Yes, since it can affect memory size used by the device, and can affect
>> performance.
>>>>> + * @param socket_id
>>>>> + *   The *socket_id* argument is the socket identifier in case of NUMA.
>>>>> + *   The value can be *SOCKET_ID_ANY* if there is no NUMA constraint
>>>> for
>>>>> + *   the DMA memory allocated for the receive descriptors of the ring.
>>>> Is it still required to be provided for hairpin Rx queue?
>>>>
>>> Yes, for internal PMD structures to be allocated, but we can if pressed
>> remove it.
>>>>> + * @param rx_conf
>>>>> + *   The pointer to the configuration data to be used for the receive
>>>> queue.
>>>>> + *   NULL value is allowed, in which case default RX configuration
>>>>> + *   will be used.
>>>>> + *   The *rx_conf* structure contains an *rx_thresh* structure with the
>>>> values
>>>>> + *   of the Prefetch, Host, and Write-Back threshold registers of the
>>>> receive
>>>>> + *   ring.
>>>>> + *   In addition it contains the hardware offloads features to activate using
>>>>> + *   the DEV_RX_OFFLOAD_* flags.
>>>>> + *   If an offloading set in rx_conf->offloads
>>>>> + *   hasn't been set in the input argument eth_conf->rxmode.offloads
>>>>> + *   to rte_eth_dev_configure(), it is a new added offloading, it must be
>>>>> + *   per-queue type and it is enabled for the queue.
>>>>> + *   No need to repeat any bit in rx_conf->offloads which has already been
>>>>> + *   enabled in rte_eth_dev_configure() at port level. An offloading
>>>> enabled
>>>>> + *   at port level can't be disabled at queue level.
>>>> Which offloads still make sense in the case of hairpin Rx queue?
>>>> What about threshhods, drop enable?
>>>>
>>> Drop and thresholds make sense, for example the application can state that,
>>> in case of back pressure to start dropping packets in order not to affect the
>>> entire nic.
>>> regarding offloads mainly vlan strip or vlan insert but those can also
>>> be used in rte_flow.
>>> But future offloads like QoS or other maybe shared.
>> I'm not a fan of dead parameters which are added just to use
>> the same structure. It raises too many questions on maintenance.
>> Also I don't like idea to share hairpin and regular offloads.
>> May be it is OK to share namespace (still unsure), but capabilities
>> are definitely different and some regular offloads are simply not
>> applicable to hairpin case.
>>
> I agree with you I think that my suggestion above (new caps for hairpin)
> solve this issue. Do you agree?
> I will remove the rte_eth_txconf and only hae the hairpin_conf with some new
> fields, same for the Rx, is that O.K.?

I think it would be better to keep only used parameters.
Anyway, it is experimental API and we can add missing parameters
when required.

[snip]

Thanks,
Andrew.