Re: [PATCH v4 4/4] PCI: Limit pci_alloc_irq_vectors() to housekeeping CPUs

From: Nitesh Narayan Lal <nitesh@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Marcelo Tosatti <mtosatti@redhat.com>,
	helgaas@kernel.org, Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	linux-pci@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
	frederic@kernel.org, sassmann@redhat.com,
	jesse.brandeburg@intel.com, lihong.yang@intel.com,
	jeffrey.t.kirsher@intel.com, jacob.e.keller@intel.com,
	jlelli@redhat.com, hch@infradead.org, bhelgaas@google.com,
	mike.marciniszyn@intel.com, dennis.dalessandro@intel.com,
	thomas.lendacky@amd.com, jiri@nvidia.com, mingo@redhat.com,
	juri.lelli@redhat.com, vincent.guittot@linaro.org,
	lgoncalv@redhat.com
Subject: Re: [PATCH v4 4/4] PCI: Limit pci_alloc_irq_vectors() to housekeeping CPUs
Date: Fri, 23 Oct 2020 09:10:41 -0400	[thread overview]
Message-ID: <9ee77056-ef02-8696-5b96-46007e35ab00@redhat.com> (raw)
In-Reply-To: <20201023085826.GP2611@hirez.programming.kicks-ass.net>

[-- Attachment #1.1: Type: text/plain, Size: 2123 bytes --]

On 10/23/20 4:58 AM, Peter Zijlstra wrote:
> On Thu, Oct 22, 2020 at 01:47:14PM -0400, Nitesh Narayan Lal wrote:
>
>> Hi Peter,
>>
>> So based on the suggestions from you and Thomas, I think something like the
>> following should do the job within pci_alloc_irq_vectors_affinity():
>>
>> +       if (!pci_is_managed(dev) && (hk_cpus < num_online_cpus()))
>> +               max_vecs = clamp(hk_cpus, min_vecs, max_vecs);
>>
>> I do know that you didn't like the usage of "hk_cpus < num_online_cpus()"
>> and to an extent I agree that it does degrade the code clarity.
> It's not just code clarity; I simply don't understand it. It feels like
> a band-aid that breaks thing.
>
> At the very least it needs a ginormous (and coherent) comment that
> explains:
>
>  - the interface
>  - the usage
>  - this hack

That make sense.

>
>> However, since there is a certain inconsistency in the number of vectors
>> that drivers request through this API IMHO we will need this, otherwise
>> we could cause an impact on the drivers even in setups that doesn't
>> have any isolated CPUs.
> So shouldn't we then fix the drivers / interface first, to get rid of
> this inconsistency?
>

Considering we agree that excess vector is a problem that needs to be
solved across all the drivers and that you are comfortable with the other
three patches in the set. If I may suggest the following:

- We can pick those three patches for now, as that will atleast fix a
  driver that is currently impacting RT workloads. Is that a fair
  expectation?

- In the meanwhile, I will start looking into individual drivers that
  consume this API to find out if there is a co-relation that can be
  derived between the max_vecs and number of CPUs. If that exists then I
  can go ahead and tweak the API's max_vecs accordingly. However, if this
  is absolutely random then I can come up with a sane comment
  before this check that covers the list of items you suggested.

- I also want to explore the comments made by Thomas which may take
  some time.

-- 
Thanks
Nitesh

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]