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

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi Ori,

On 10/29/19 10:39 PM, Ori Kam wrote:
>> On 10/28/19 9:44 PM, Ori Kam wrote:
>>>> On 10/27/19 3:24 PM, Ori Kam wrote:
>>>>> +			if (rte_eth_dev_is_rx_hairpin_queue(dev, i))
>>>> The condition should be more tricky if we resetup hairpin queue.
>>>> I.e. we should check if i is rx_queue_id and count it anyway.
>>>>
>>>>> +				count++;
>>>>> +		}
>>>>> +		if (count > cap.max_nb_queues) {
>>>>> +			RTE_ETHDEV_LOG(ERR, "To many Rx hairpin queues
>>>> %d",
>>>>
>>>> I think it would be useful to log max here as well to catch
>>>> unset max cases easier.
>>>>
>>> I'm not sure I understand.
>> If the question is about logging, the answer is simple:
>> if the user forget to initialize maximum number of hairpin queues
>> properly, it will be zero and setup will fail here. So, it would be
>> good to log maximum value here just to make it clear which
>> limit is exceeded.
>>
> Maybe I'm missing something but the PMD sets the max number of hairpin queues.

Yes, it is just my paranoia to simplify debugging the case if PMD 
forgets to do it.

> But in any case I agree we should log what the user requested and what is the max
> that the PMD reports.
>
>> If the question is about above check, let's consider the case when
>> maximum is one and one hairpin queue is already setup, but
>> user tries to setup one more. Above loop will count only one since
>> hairpin state for current queue is set below. So, the condition will
>> allow to setup the second hairpin queue.
>> In theory, we could initialize cound=1 to count this one, but
>> it would break the case when we call setup once again for the
>> queue which is already hairpin. API allows and handles it.
>>
> Nice catch. I think the best solution is to compare the count to cap.max_nb_queues - 1.
> and even before this comparison check if the current queue is already hairpin queue if so
> we can skip this check.
> What do you think?

I think the right solution is always count current queue since it is either
becoming hairpin or already hairpin, i.e.

if (i == rx_queue_id || rte_eth_dev_is_rx_hairpin_queue(dev, i))

So, the result will be always total number of hairpin queues if
this one is hairpin.

Andrew.