Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d3b32b23c79f78ea99316bae77fab2064833d8e7.camel@redhat.com>
Subject: Re: [RFC PATCH 0/5] irq: sysfs interface improvements for SMP
 affinity control
From:   Radu Rendec <rrendec@redhat.com>
To:     Thomas Gleixner <tglx@linutronix.de>, Marc Zyngier <maz@kernel.org>
Cc:     linux-kernel@vger.kernel.org
Date:   Tue, 20 Jun 2023 11:58:11 -0400
In-Reply-To: <877csohcll.ffs@tglx>
References: <877csohcll.ffs@tglx>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-05-31 at 15:09 +0200, Thomas Gleixner wrote:
> On Tue, May 30 2023 at 17:45, Radu Rendec wrote:
> > Note: I still need to update the Documentation/ directory for the new
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sysfs interface, and I will address that=
 in a future version.
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 At this point, I just want to get feedba=
ck about the current
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 approach.
>=20
> From a conceptual POV I understand why this is required, which makes me
> hate this chained mechanism even more.
>=20
> Aside of having no visibility (counters, affinity, etc) the worst thing
> about these chained hidden interrupts is:
>=20
> =C2=A0 There is no control of runaway interrupts as they circumvent the c=
ore,
> =C2=A0 which has caused hard to diagnose system hangups in the past. See
> =C2=A0=C2=A0=20
> =C2=A0=C2=A0=C2=A0 ba714a9c1dea ("pinctrl/amd: Use regular interrupt inst=
ead of chained")
>=20
> =C2=A0 for demonstration.
>=20
> The argument I heard for this chained interrupt muck is that it's so
> much more performant than using regular interrupt handlers via
> request_irq(). It's obviously less overhead, but whether it matters and
> most importantly whether it justifies the downsides is a different
> question.
>=20
> There is also the argument about double accounting. Right now the
> chained handler is not accounted and only the childs are.
>=20
> Though that is inconsistent with other demultiplex handlers which _must_
> use regular interrupt handlers (threaded) to demultiplex interrupt chips
> which sit behind SPI/I2C...
>=20
> The sum of child interrupts is also not necessarily the number of parent
> interrupts, unless there is always exactly only one child handler to
> invoke.
>=20
> Quite some of those demux handlers are also not RT compatible.
>=20
> AFAICT, there is no real technical reason at least not for the vast
> majority of usage sites why the demultiplex handler cannot run inside a
> regular interrupt handler context.

I was about to send an RFC patch that converts a multiplexing IRQ chip
driver from chained to regular interrupts, when I realized I had come
full circle. Marc rejected a similar patch in the past [1] and the main
argument was that exposing the parent interrupt in procfs is backwards
incompatible. Quote:

 The problem of changing affinities for chained (or multiplexing)
 interrupts is, to make it short, that it breaks the existing userspace
 ABI that a change in affinity affects only the interrupt userspace
 acts upon, and no other. Which is why we don't expose any affinity
 setting for such an interrupt, as by definition changing its affinity
 affects all the interrupts that are muxed onto it.

So, there are two contradictory arguments here:
 * Chained interrupts are "muck", mainly because they circumvent the
   interrupt core and prevent the interrupt storm detector from kicking
   in when hardware misbehaves (and for other reasons as well).
 * Exposing the parent interrupt affinity control in procfs breaks the
   userspace ABI because all child interrupts inherently share the same
   affinity settings. This implies regular interrupts cannot be used.

Meanwhile, both interrupt storms and the lack of affinity control
support for multiplexing drivers are real problems, and my team and I
came across both. FWIW, the interrupt storm is the one we found more
recently, and it's the reason why I wanted to send that RFC patch I
mentioned before.

Is Marc's argument still relevant? Perhaps with both arguments in the
same email it becomes clearer that there needs to be some alignment at
the maintainer level. I would be more than happy to send patches and
help fix both issues. But I think that's not possible until you come up
with a strategy that you *both* agree to.

[1] https://lore.kernel.org/all/874k0bf7f7.wl-maz@kernel.org/

Thanks,
Radu

> So I personally would prefer to get rid of this chained oddball and just
> have consistent mechanisms for dealing with interrupts, which would just
> avoid exposing the affinity files in two different places.
>=20
> Providing information about child/parent relationship is an orthogonal
> issue.
>=20
> If there is some good reason (aside of the chained muck) to have sysfs
> based affinity management, then I'm not objecting as long as the
> functionality is the same, i.e. effective affinity needs be exposed too.
>=20
> Thanks,
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tglx