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

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi Ori,

On 10/16/19 10:36 PM, Ori Kam wrote:
> Hi Andrew,
>
> Thanks again for your time.
>
> PSB,
> Ori
>
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Tuesday, October 15, 2019 1:12 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: [PATCH v3 01/14] ethdev: add support for hairpin queue
>>
>> Hi Ori,
>>
>> On 10/15/19 12:04 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>

[snip]

>>> @@ -746,9 +769,9 @@ struct rte_eth_dev_data {
>>>    		dev_started : 1,   /**< Device state: STARTED(1) / STOPPED(0). */
>>>    		lro         : 1;   /**< RX LRO is ON(1) / OFF(0) */
>>>    	uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>> -			/**< Queues state: STARTED(1) / STOPPED(0). */
>>> +		/**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
>>>    	uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>> -			/**< Queues state: STARTED(1) / STOPPED(0). */
>>> +		/**< Queues state: HAIRPIN(2) STARTED(1) / STOPPED(0). */
>> / is missing above after HAIRPIN(2).
>> In fact there is no point to duplicate values in parenthesis, but it is
>> out of scope of the review.
>>
> I will add the missing /
>
>> I'm not 100% happy that it makes impossible to mark hairpin queues
>> as started/stopped. It is not that important right now, but may be it is
>> better to use state as bit field. Bit 0 - stopped/started,
>> bit 1 - regular/hairpin. Anyway, it is internal interface.
>>
> Your idea is very nice, but there are some things to consider.
> For example if converted to bits it will take more memory.
> We can just it is flags for the U8 but this will mean that we could have
> both hairpin and stopped / started in the same time. Which I'm not sure if
> it is good or bad. Like you say it is internal so let's keep the current implementation,
> and discuss your idea after this patch set, and after we will see how the community uses
> the hairpin feature. Is that O.K. for you?

Yes.