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

[Ori Kam <orika@mellanox.com>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Wednesday, October 30, 2019 8:39 AM
> To: Ori Kam <orika@mellanox.com>; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.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 v6 02/14] ethdev: add support for hairpin queue
> 
> 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.
> 

Paranoia is a good thing.
I will add the logging.

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

Good Idea will implement it.

Thanks,
Ori