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>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	helgaas@kernel.org, Thomas Gleixner <tglx@linutronix.de>
Cc: 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, Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH v4 4/4] PCI: Limit pci_alloc_irq_vectors() to housekeeping CPUs
Date: Thu, 22 Oct 2020 13:47:14 -0400	[thread overview]
Message-ID: <260f4191-5b9f-6dc1-9f11-085533ac4f55@redhat.com> (raw)
In-Reply-To: <6736e643-d4ae-9919-9ae1-a73d5f31463e@redhat.com>

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

On 10/20/20 10:39 AM, Nitesh Narayan Lal wrote:
> On 10/20/20 9:41 AM, Peter Zijlstra wrote:
>> On Tue, Oct 20, 2020 at 09:00:01AM -0400, Nitesh Narayan Lal wrote:
>>> On 10/20/20 3:30 AM, Peter Zijlstra wrote:
>>>> On Mon, Oct 19, 2020 at 11:00:05AM -0300, Marcelo Tosatti wrote:
>>>>>> So I think it is important to figure out what that driver really wants
>>>>>> in the nohz_full case. If it wants to retain N interrupts per CPU, and
>>>>>> only reduce the number of CPUs, the proposed interface is wrong.
>>>>> It wants N interrupts per non-isolated (AKA housekeeping) CPU.
>>>> Then the patch is wrong and the interface needs changing from @min_vecs,
>>>> @max_vecs to something that expresses the N*nr_cpus relation.
>>> Reading Marcelo's comment again I think what is really expected is 1
>>> interrupt per non-isolated (housekeeping) CPU (not N interrupts).
>> Then what is the point of them asking for N*nr_cpus when there is no
>> isolation?
>>
>> Either everybody wants 1 interrupts per CPU and we can do the clamp
>> unconditionally, in which case we should go fix this user, or they want
>> multiple per cpu and we should go fix the interface.
>>
>> It cannot be both.
> Based on my understanding I don't think this is consistent, the number
> of interrupts any driver can request varies to an extent that some
> consumer of this API even request just one interrupt for its use.
>
> This was one of the reasons why I thought of having a conditional
> restriction.
>
> But I agree there is a lack of consistency.
>

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.

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.

If you agree, I can send the next version of the patch-set.

-- 
Thanks
Nitesh

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