Hi Andrew,
> -----Original Message-----
> From: Andrew Rybchenko <arybche...@solarflare.com>
> Sent: Wednesday, October 30, 2019 8:39 AM
> To: Ori Kam <or...@mellanox.com>; John McNamara
> <john.mcnam...@intel.com>; Marko Kovacevic
> <marko.kovace...@intel.com>; Thomas Monjalon <tho...@monjalon.net>;
> Ferruh Yigit <ferruh.yi...@intel.com>
> Cc: dev@dpdk.org; jingjing...@intel.com; step...@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
