Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Long Li <longli@microsoft.com>
To:     Thomas Gleixner <tglx@linutronix.de>
CC:     LKML <linux-kernel@vger.kernel.org>,
        Michael Kelley <mikelley@microsoft.com>
Subject: RE: [PATCH] Choose CPU based on allocated IRQs
Thread-Topic: [PATCH] Choose CPU based on allocated IRQs
Thread-Index: AQHUanGopgv0yEp/J0aFaWDzsmj8YaU2x/KAgAFONDA=
Date:   Tue, 30 Oct 2018 17:44:54 +0000
Message-ID: <MWHPR2101MB0729230F08FA352EDAB9C486CECC0@MWHPR2101MB0729.namprd21.prod.outlook.com>
References: <20181023014044.15888-1-longli@linuxonhyperv.com>
 <alpine.DEB.2.21.1810292138480.5984@nanos.tec.linutronix.de>
In-Reply-To: <alpine.DEB.2.21.1810292138480.5984@nanos.tec.linutronix.de>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: microsoft.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6448b062-46f6-4319-b4fc-08d63e8f6709
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Oct 2018 17:44:54.7627
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR2101MB0811
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH] Choose CPU based on allocated IRQs
>=20
> Long,
>=20
> On Tue, 23 Oct 2018, Long Li wrote:
>=20
> thanks for this patch.
>=20
> A trivial formal thing ahead. The subject line
>=20
>    [PATCH] Choose CPU based on allocated IRQs
>=20
> is lacking a proper subsystem prefix. In most cases you can figure the pr=
efix
> out by running 'git log path/to/file' which in this case will show you th=
at most
> commits touching this file use the prefix 'genirq/matrix:'.
>=20
> So the proper subject would be:
>=20
>    [PATCH] genirq/matrix: Choose CPU based on allocated IRQs
>=20
> Subsystem prefixes are important to see where a patch belongs to right fr=
om
> the subject. Without that it could belong to any random part of the kerne=
l
> and needs further inspection of the patch itself. This applies to both em=
ail
> and to git shortlog listings.

Thank you. I will send v2 to address this.

>=20
> > From: Long Li <longli@microsoft.com>
> >
> > In irq_matrix, "available" is set when IRQs are allocated earlier in
> > the IRQ assigning process.
> >
> > Later, when IRQs are activated those values are not good indicators of
> > what CPU to choose to assign to this IRQ.
>=20
> Can you please explain why you think that available is the wrong indicato=
r
> and which problem you are trying to solve?
>=20
> The WHY is really the most important part of a changelog.

The problem I'm seeing is that on a very large system with multiple devices=
 of the same class (e.g. NVMe disks, using managed IRQs), they tend to use =
interrupts on several CPUs on the system. Under heavy load, those several C=
PUs are busy while other CPU are most idling. The issue is that when NVMe c=
all irq_matrix_alloc_managed(), the assigned the CPU is always the first CP=
U in the cpumask, because they check for cpumap->available that will not ch=
ange after managed IRQs are reserved in irq_matrix_reserve_managed (which w=
as called from the 1st stage of IRQ setup in irq_domain_ops->alloc).

>=20
> > Change to choose CPU for an IRQ based on how many IRQs are already
> > allocated on this CPU.
>=20
> Looking deeper. The initial values are:
>=20
>     available =3D alloc_size - (managed + systembits)
>     allocated =3D 0
>=20
> There are two distinct functionalities which modify 'available' and 'allo=
cated'
> (omitting the reverse operations for simplicity):
>=20
> 1) managed interrupts
>=20
>    reserve_managed()
> 	managed++;
> 	available--;
>=20
>    alloc_managed()
>         allocated++;
>=20
> 2) regular interrupts
>=20
>    alloc()
> 	allocated++;
> 	available--;
>=20
> So 'available' can be lower than 'allocated' depending on the number of
> reserved managed interrupts, which have not yet been activated.
>=20
> So for all regular interrupts we really want to look at the number of 'av=
ailable'
> vectors because the reserved managed ones are already accounted there
> and they need to be taken into account.

I think "reserved managed" may not always be accurate. Reserved managed IRQ=
s may not always get activated. For an irq_data, when irq_matrix_reserve_ma=
naged is called, all the CPUs in the cpumask are reserved. Later, only one =
of them is activated via the call to irq_matrix_alloc_managed(). So we end =
up with a number of "reserved managed" that never get used.

>=20
> For the spreading of managed interrupts in alloc_managed() that's indeed =
a
> different story and 'allocated' is more correct. But even that is not com=
pletely
> accurate and can lead to the wrong result. The accurate solution would be=
 to
> account the managed _and_ allocated vectors separately and do the
> spreading for managed interrupts based on that.

I think checking for "allocated" is the best approach for picking which CPU=
 to assign for a given irq_data, since we really can't rely on "managed" to=
 decide how busy this CPU really is. Checking for "allocated" should work f=
or both unmanaged and managed IRQs.

>=20
> Thanks,
>=20
> 	tglx