Message-ID: <176efdda-7d23-dea2-ef41-5df7cae56233@redhat.com>
Date:   Thu, 24 Sep 2020 19:40:49 -0400
From:   Nitesh Narayan Lal <nitesh@...hat.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-pci@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
        frederic@...nel.org, mtosatti@...hat.com, sassmann@...hat.com,
        jesse.brandeburg@...el.com, lihong.yang@...el.com,
        jeffrey.t.kirsher@...el.com, jacob.e.keller@...el.com,
        jlelli@...hat.com, hch@...radead.org, bhelgaas@...gle.com,
        mike.marciniszyn@...el.com, dennis.dalessandro@...el.com,
        thomas.lendacky@....com, jerinj@...vell.com,
        mathias.nyman@...el.com, jiri@...dia.com, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org
Subject: Re: [PATCH v2 4/4] PCI: Limit pci_alloc_irq_vectors as per
 housekeeping CPUs


On 9/24/20 6:59 PM, Bjorn Helgaas wrote:
> On Thu, Sep 24, 2020 at 05:39:07PM -0400, Nitesh Narayan Lal wrote:
>> On 9/24/20 4:45 PM, Bjorn Helgaas wrote:
>>> Possible subject:
>>>
>>>   PCI: Limit pci_alloc_irq_vectors() to housekeeping CPUs
>> Will switch to this.
>>
>>> On Wed, Sep 23, 2020 at 02:11:26PM -0400, Nitesh Narayan Lal wrote:
>>>> This patch limits the pci_alloc_irq_vectors, max_vecs argument that is
>>>> passed on by the caller based on the housekeeping online CPUs (that are
>>>> meant to perform managed IRQ jobs).
>>>>
>>>> A minimum of the max_vecs passed and housekeeping online CPUs is derived
>>>> to ensure that we don't create excess vectors as that can be problematic
>>>> specifically in an RT environment. In cases where the min_vecs exceeds the
>>>> housekeeping online CPUs, max vecs is restricted based on the min_vecs
>>>> instead. The proposed change is required because for an RT environment
>>>> unwanted IRQs are moved to the housekeeping CPUs from isolated CPUs to
>>>> keep the latency overhead to a minimum. If the number of housekeeping CPUs
>>>> is significantly lower than that of the isolated CPUs we can run into
>>>> failures while moving these IRQs to housekeeping CPUs due to per CPU
>>>> vector limit.
>>> Does this capture enough of the log?
>>>
>>>   If we have isolated CPUs dedicated for use by real-time tasks, we
>>>   try to move IRQs to housekeeping CPUs to reduce overhead on the
>>>   isolated CPUs.
>> How about:
>> "
>> If we have isolated CPUs or CPUs running in nohz_full mode for the purpose
>> of real-time, we try to move IRQs to housekeeping CPUs to reduce latency
>> overhead on these real-time CPUs.
>> "
>>
>> What do you think?
> It's OK, but from the PCI core perspective, "nohz_full mode" doesn't
> really mean anything.  I think it's a detail that should be inside the
> "housekeeping CPU" abstraction.

I get your point, in that case I will probably stick to your original
suggestion just replacing the term "overhead" with "latency overhead".

>
>>>   If we allocate too many IRQ vectors, moving them all to housekeeping
>>>   CPUs may exceed per-CPU vector limits.
>>>
>>>   When we have isolated CPUs, limit the number of vectors allocated by
>>>   pci_alloc_irq_vectors() to the minimum number required by the
>>>   driver, or to one per housekeeping CPU if that is larger
>> I think this is good, I can adopt this.
>>
>>> .
>>>
>>>> Signed-off-by: Nitesh Narayan Lal <nitesh@...hat.com>
>>>> ---
>>>>  include/linux/pci.h | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 835530605c0d..cf9ca9410213 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -38,6 +38,7 @@
>>>>  #include <linux/interrupt.h>
>>>>  #include <linux/io.h>
>>>>  #include <linux/resource_ext.h>
>>>> +#include <linux/sched/isolation.h>
>>>>  #include <uapi/linux/pci.h>
>>>>  
>>>>  #include <linux/pci_ids.h>
>>>> @@ -1797,6 +1798,20 @@ static inline int
>>>>  pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>>>>  		      unsigned int max_vecs, unsigned int flags)
>>>>  {
>>>> +	unsigned int hk_cpus = hk_num_online_cpus();
>>>> +
>>>> +	/*
>>>> +	 * For a real-time environment, try to be conservative and at max only
>>>> +	 * ask for the same number of vectors as there are housekeeping online
>>>> +	 * CPUs. In case, the min_vecs requested exceeds the housekeeping
>>>> +	 * online CPUs, restrict the max_vecs based on the min_vecs instead.
>>>> +	 */
>>>> +	if (hk_cpus != num_online_cpus()) {
>>>> +		if (min_vecs > hk_cpus)
>>>> +			max_vecs = min_vecs;
>>>> +		else
>>>> +			max_vecs = min_t(int, max_vecs, hk_cpus);
>>>> +	}
>>> Is the below basically the same?
>>>
>>> 	/*
>>> 	 * If we have isolated CPUs for use by real-time tasks,
>>> 	 * minimize overhead on those CPUs by moving IRQs to the
>>> 	 * remaining "housekeeping" CPUs.  Limit vector usage to keep
>>> 	 * housekeeping CPUs from running out of IRQ vectors.
>>> 	 */
>> How about the following as a comment:
>>
>> "
>> If we have isolated CPUs or CPUs running in nohz_full mode for real-time,
>> latency overhead is minimized on those CPUs by moving the IRQ vectors to
>> the housekeeping CPUs. Limit the number of vectors to keep housekeeping
>> CPUs from running out of IRQ vectors.
>>
>> "
>>
>>> 	if (housekeeping_cpus < num_online_cpus()) {
>>> 		if (housekeeping_cpus < min_vecs)
>>> 			max_vecs = min_vecs;
>>> 		else if (housekeeping_cpus < max_vecs)
>>> 			max_vecs = housekeeping_cpus;
>>> 	}
>> The only reason I went with hk_cpus instead of housekeeping_cpus is because
>> at other places in the kernel I found a similar naming convention (eg.
>> hk_mask, hk_flags etc.).
>> But if housekeeping_cpus makes things more clear, I can switch to that
>> instead.
>>
>> Although after Frederic and Peter's suggestion the previous call will change
>> to something like:
>>
>> "
>> housekeeping_cpus = housekeeping_num_online_cpus(HK_FLAG_MANAGED_IRQ);
>> "
>>
>> Which should still falls in the that 80 chars per line limit.
> I don't really care whether it's "hk_cpus" or "housekeeping_cpus" as
> long as "housekeeping" appears in the code somewhere.  If we call
> "housekeeping_num_online_cpus()" that should be enough.

Got it, in that case we are good here.

>
>>> My comment isn't quite right because this patch only limits the number
>>> of vectors; it doesn't actually *move* IRQs to the housekeeping CPUs.
>> Yeap it doesn't move IRQs to the housekeeping CPUs.
>>
>>> I don't know where the move happens (or maybe you just avoid assigning
>>> IRQs to isolated CPUs, and I don't know how that happens either).
>> This can happen in the userspace, either manually or by some application
>> such as tuned.
> Some brief hint about this might be helpful.

Sure, I will try to come up with something on the lines what you suggested
i.e., it also includes the information that the IRQs are moved from the
userspace.


>
>>>>  	return pci_alloc_irq_vectors_affinity(dev, min_vecs, max_vecs, flags,
>>>>  					      NULL);
>>>>  }
>>>> -- 
>>>> 2.18.2
>>>>
>> -- 
>> Thanks
>> Nitesh
>>
>
>
-- 
Thanks
Nitesh



Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists