Hi folks!
This is the spiritual successor to [1], which was over 6 years ago (!).
Background
==========
GIC mechanics
+++++++++++++
There are three IRQ operations:
o Acknowledge. This gives us the IRQ number that interrupted us, and also
- raises the running priority of the CPU interface to that of the IRQ
- sets the active bit of the IRQ
o Priority Drop. This "clears" the running priority.
o Deactivate. This clears the active bit of the IRQ.
o The CPU interface has a running priority value. No interrupt of lower or
equal priority will be signaled to the CPU attached to that interface. On
Linux, we only have two priority values: pNMIs at highest priority, and
everything else at the other priority.
o Most GIC interrupts have an "active" bit. This bit is set on Acknowledge
and cleared on Deactivate. A given interrupt cannot be re-signaled to a
CPU if it has its active bit set (i.e. if it "fires" again while it's
being handled).
EOImode fun
+++++++++++
In EOImode=0, Priority Drop and Deactivate are undissociable. The
(simplified) interrupt handling flow is as follows:
<~IRQ>
Acknowledge
Priority Drop + Deactivate
<interrupts can once again be signaled, once interrupts are re-enabled>
With EOImode=1, we can invoke each operation individually. This gives us:
<~IRQ>
Acknowledge
Priority Drop
<*other* interrupts can be signaled from here, once interrupts are re-enabled>
Deactivate
<*this* interrupt can be signaled again>
What this means is that with EOImode=1, any interrupt is kept "masked" by
its active bit between Priority Drop and Deactivate.
Threaded IRQs and ONESHOT
=========================
ONESHOT threaded IRQs must remain masked between the main handler and the
threaded handler. Right now we do this using the conventional irq_mask()
operations, which looks like this:
<irq handler>
Acknowledge
Priority Drop
irq_mask()
Deactivate
<threaded handler>
irq_unmask()
However, masking for the GICs means poking the distributor, and there's no
sysreg for that - it's an MMIO access. We've seen above that our IRQ
handling can give us masking "for free", and this is what this patch set is
all about. It turns the above handling into:
<irq handler>
Acknowledge
Priority Drop
<threaded handler>
Deactivate
No irq_mask() => fewer MMIO accesses => happier users (or so I've been
told). This is especially relevant to PREEMPT_RT which forces threaded
IRQs.
Testing
=======
GICv2
+++++
I've tested this on my Juno with forced irqthreads. This makes the pl011
IRQ into a threaded ONESHOT IRQ, so I spammed my keyboard into the console
and verified via ftrace that there were no irq_mask() / irq_unmask()
involved.
GICv3
+++++
I've tested this on my Ampere eMAG, which uncovered "fun" interactions with
the MSI domains. Did the same trick as the Juno with the pl011.
pNMIs cause said eMAG to freeze, but that's true even without my patches. I
did try them out under QEMU+KVM and that looked fine, although that means I
only got to test EOImode=0. I'll try to dig into this when I get some more
cycles.
TODOs
=====
MSI hierarchy story
+++++++++++++++++++
I 'hate' patch 8/10 because it's so very easy to break. See changelog for
my thoughts on the thing.
Performance numbers
+++++++++++++++++++
The whole point of this was to shave off some time spent issuing requests
to the distributor. On my Juno the IRQ for eth0 gets forcefully threaded
and thus becomes a ONESHOT threaded IRQ - I've tried to test this with
netperf, but the delta is lost in the noise (and there is quite a lot of
noise, unfortunately).
I'm still looking for something I can benchmark on the eMAG to get some
GICv3 results.
Links
=====
[1]: https://lore.kernel.org/lkml/1414235215-10468-1-git-send-email-marc.zyngier@arm.com/
Valentin Schneider (10):
genirq: Add chip flag to denote automatic IRQ (un)masking
genirq: Define irq_ack() and irq_eoi() helpers
genirq: Employ ack_irq() and eoi_irq() where relevant
genirq: Add handle_strict_flow_irq() flow handler
genirq: Let purely flow-masked ONESHOT irqs through
unmask_threaded_irq()
genirq: Don't mask IRQ within flow handler if IRQ is flow-masked
genirq, irq-gic-v3: Make NMI flow handlers use ->irq_ack() if
available
irqchip/gic-v3-its: Use irq_chip_ack_parent()
irqchip/gic: Convert to handle_strict_flow_irq()
irqchip/gic-v3: Convert to handle_strict_flow_irq()
drivers/irqchip/irq-gic-v3-its-pci-msi.c | 1 +
drivers/irqchip/irq-gic-v3-its.c | 1 +
drivers/irqchip/irq-gic-v3.c | 27 ++++--
drivers/irqchip/irq-gic.c | 14 ++-
include/linux/irq.h | 15 ++-
kernel/irq/chip.c | 117 +++++++++++++++++++----
kernel/irq/debugfs.c | 2 +
kernel/irq/internals.h | 7 ++
kernel/irq/manage.c | 2 +-
9 files changed, 154 insertions(+), 32 deletions(-)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Some IRQ chips such as the Arm GICs automagically mask / unmask an
IRQ during the handling of said IRQ. This renders further mask / unmask
operations within the flow handlers redundant, which we do want to leverage
as masking by itself is not cheap (Distributor access via MMIO for GICs).
This is different from having a chip->irq_mask_ack() callback as this
masking is:
- inherent to the chip->irq_ack() and *cannot* be omitted
- a *different* masking state than chip->irq_mask() (chip->irq_mask() is
idempotent, chip->irq_ack() really isn't)
Add a chip flag, IRQCHIP_AUTOMASKS_FLOW, to denote chips with such
behaviour. Add a new IRQ data flag, IRQD_IRQ_FLOW_MASKED, to keep this
flow-induced mask state separate from regular mask / unmask operations
(IRQD_IRQ_MASKED).
Signed-off-by: Valentin Schneider <redacted>
---
include/linux/irq.h | 10 ++++++++++
kernel/irq/chip.c | 5 +++++
kernel/irq/debugfs.c | 2 ++
kernel/irq/internals.h | 5 +++++
4 files changed, 22 insertions(+)
The newly-added IRQCHIP_AUTOMASKS_FLOW flag requires some additional
bookkeeping around chip->{irq_ack, irq_eoi}() calls. Define wrappers around
those chip callbacks to drive the IRQD_IRQ_FLOW_MASKED state of an IRQ when
the chip has the IRQCHIP_AUTOMASKS_FLOW flag.
Signed-off-by: Valentin Schneider <redacted>
---
kernel/irq/chip.c | 16 ++++++++++++++++
kernel/irq/internals.h | 2 ++
2 files changed, 18 insertions(+)
This can easily be coccinelle'd to replace all existing chip->irq_{ack,
eoi} callsites, however not all callsites benefit from this
replacement: fasteoi flow handlers for instance only deal with an
->irq_eoi() callback. Instead, only patch callsites that can benefit from
the added functionality.
Signed-off-by: Valentin Schneider <redacted>
---
kernel/irq/chip.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
The GIC family of irqchips have been so far treated as "fasteoi"
chips. As handle_fasteoi_irq() states, this implies:
* Only a single callback will be issued to the chip: an ->eoi()
* call when the interrupt has been serviced.
However, the GICs have an operating mode (EOImode=1) which requires an
additional chip interaction during the IRQ handling. Said operating mode
already has some uses with virtualization, but could also be leveraged to
slightly optimize ONESHOT IRQs.
This extra interaction is currently hidden away in the drivers, but further
exploiting its effects (see IRQD_IRQ_FLOW_MASKED) requires lifting it from
the driver code into core code. It so happens that it fits the role of
->irq_ack(); unfortunately, the GICs require both callbacks to be strictly
paired with one another: for a given IRQ activation, there must be a single
->irq_ack() followed by a single ->irq_eoi(). No more, no less, and in that
order.
Introduce a new flow handler which guarantees said ack / eoi pairing. Note
that it is strikingly similar to handle_fasteoi_mask_irq() for now, but
will be further modified in later patches
Signed-off-by: Valentin Schneider <redacted>
---
include/linux/irq.h | 1 +
kernel/irq/chip.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+)
A subsequent patch will let IRQs end up in irq_finalize_oneshot() without
IRQD_IRQ_MASKED, but with IRQD_IRQ_FLOW_MASKED set instead. Let such IRQs
receive their final ->irq_eoi().
Signed-off-by: Valentin Schneider <redacted>
---
kernel/irq/manage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
mask_irq() lets an IRQ with IRQD_IRQ_FLOW_MASKED set be further masked via
chip->irq_mask(). This is necessary for unhandled IRQs as we want to keep
them masked beyond eoi_irq() (which clears IRQD_IRQ_FLOW_MASKED).
This is however not necessary in paths that do end up handling the IRQ and
are bounded by a final eoi_irq() - this is the case for chips with
IRQCHIP_AUTOMASKS_FLOW and IRQCHIP_EOI_THREADED.
Make handle_strict_flow_irq() leverage IRQCHIP_AUTOMASKS_FLOW and issue an
ack_irq() rather than a mask_ack_irq() when possible.
Signed-off-by: Valentin Schneider <redacted>
---
kernel/irq/chip.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
Subsequent patches will make the gic-v3 irqchip use an ->irq_ack()
callback. As a preparation, make the NMI flow handlers call said callback
if it is available.
Since this departs from the fasteoi scheme of only issuing a suffix
->eoi(), rename the NMI flow handlers.
Signed-off-by: Valentin Schneider <redacted>
---
drivers/irqchip/irq-gic-v3.c | 4 ++--
include/linux/irq.h | 4 ++--
kernel/irq/chip.c | 25 ++++++++++++++-----------
3 files changed, 18 insertions(+), 15 deletions(-)
@@ -484,10 +484,10 @@ static int gic_irq_nmi_setup(struct irq_data *d)/* Setting up PPI as NMI, only switch handler for first NMI */if(!refcount_inc_not_zero(&ppi_nmi_refs[idx])){refcount_set(&ppi_nmi_refs[idx],1);-desc->handle_irq=handle_percpu_devid_fasteoi_nmi;+desc->handle_irq=handle_percpu_devid_nmi;}}else{-desc->handle_irq=handle_fasteoi_nmi;+desc->handle_irq=handle_nmi;}gic_irq_set_prio(d,GICD_INT_NMI_PRI);
Subsequent patches will make the GIC irqchips use a flow handler that
issues an ->irq_ack(). irqchips of child domains need to handle this.
Note: I'm very much not fond of this; this is treacherous and explodes if
any parent chip doesn't have an ->ack() callback. It turns out okay with
EOImode=0 because handle_fasteoi_irq() doesn't issue any ->ack(), but that
is very fragile at best.
An alternative would be to
o make irq_chip_ack_parent() check the callback against NULL
o make irq_chip_ack_parent() the default chip->irq_ack() via
MSI_FLAG_USE_DEF_CHIP_OPS.
XXX: what about pMSI and fMSI ?
Signed-off-by: Valentin Schneider <redacted>
---
drivers/irqchip/irq-gic-v3-its-pci-msi.c | 1 +
drivers/irqchip/irq-gic-v3-its.c | 1 +
2 files changed, 2 insertions(+)
Now that the proper infrastructure is in place, convert the irq-gic chip to
use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW.
For EOImode=1, the Priority Drop is moved from gic_handle_irq() into
chip->irq_ack(). This effectively pushes the EOI write down into
->handle_irq(), but doesn't change its ordering wrt the irqaction
handling.
The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the
->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This
means a threaded ONESHOT IRQ can now be handled entirely without a single
chip->irq_mask() call.
EOImode=0 handling remains unchanged.
Signed-off-by: Valentin Schneider <redacted>
---
drivers/irqchip/irq-gic.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
Now that the proper infrastructure is in place, convert the irq-gic-v3 chip
to use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW.
For EOImode=1, the Priority Drop is moved from gic_handle_irq() into
chip->irq_ack(). This effectively pushes the EOIR write down into
->handle_irq(), but doesn't change its ordering wrt the irqaction
handling.
The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the
->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This
means a threaded ONESHOT IRQ can now be handled entirely without a single
chip->irq_mask() call.
Despite not having an Active state, LPIs are made to use
handle_strict_flow_irq() as well. This lets them re-use
gic_eoimode1_chip.irq_ack() as Priority Drop, rather than special-case them
in gic_handle_irq().
EOImode=0 handling remains unchanged.
Signed-off-by: Valentin Schneider <redacted>
---
drivers/irqchip/irq-gic-v3.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
Some IRQ chips such as the Arm GICs automagically mask / unmask an
IRQ during the handling of said IRQ. This renders further mask / unmask
operations within the flow handlers redundant, which we do want to leverage
as masking by itself is not cheap (Distributor access via MMIO for GICs).
This is different from having a chip->irq_mask_ack() callback as this
masking is:
- inherent to the chip->irq_ack() and *cannot* be omitted
- a *different* masking state than chip->irq_mask() (chip->irq_mask() is
idempotent, chip->irq_ack() really isn't)
Add a chip flag, IRQCHIP_AUTOMASKS_FLOW, to denote chips with such
behaviour. Add a new IRQ data flag, IRQD_IRQ_FLOW_MASKED, to keep this
flow-induced mask state separate from regular mask / unmask operations
(IRQD_IRQ_MASKED).
Signed-off-by: Valentin Schneider <redacted>
Hi Valentin,
One totally trivial thing noticed whilst reading.
Nitpick of the day : Seems text above is using tabs for white space blocks
whereas you have used spaces. Make it consistent.
It's not consistent in the file so I guess you could clean that up, or
just go with making it consistent in this block.
@@ -567,6 +575,7 @@ struct irq_chip { * IRQCHIP_SUPPORTS_NMI: Chip can deliver NMIs, only for root irqchips * IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND: Invokes __enable_irq()/__disable_irq() for wake irqs * in the suspend path if they are in disabled state+ * IRQCHIP_AUTOMASKS_FLOW: chip->ack() masks and chip->eoi() unmasks */ enum { IRQCHIP_SET_TYPE_MASKED = (1 << 0),
Nitpick of the day : Seems text above is using tabs for white space blocks
whereas you have used spaces. Make it consistent.
It's not consistent in the file so I guess you could clean that up, or
just go with making it consistent in this block.
I usually let my editor take the wheel when it comes to whitespace vs tabs,
but it does look like it's not aligned with the rest here.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Thomas Gleixner <hidden> Date: 2021-04-09 13:52:53
On Thu, Apr 08 2021 at 16:43, Valentin Schneider wrote:
quoted hunk
A subsequent patch will let IRQs end up in irq_finalize_oneshot() without
IRQD_IRQ_MASKED, but with IRQD_IRQ_FLOW_MASKED set instead. Let such IRQs
receive their final ->irq_eoi().
Signed-off-by: Valentin Schneider <redacted>
---
kernel/irq/manage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
This creates better code when you have '|' instead of '||' because the
compiler can spare a conditional and combine the bit check into one.
Thanks,
tglx
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Thomas Gleixner <hidden> Date: 2021-04-09 13:57:44
On Thu, Apr 08 2021 at 16:43, Valentin Schneider wrote:
+ /*
+ * Masking is required if IRQ is ONESHOT and we can't rely on the
+ * flow-masking persisting down to irq_finalize_oneshot()
+ * (in the IRQ thread).
+ */
+ if ((desc->istate & IRQS_ONESHOT) &&
+ (!(chip->flags & IRQCHIP_AUTOMASKS_FLOW) ||
+ !(chip->flags & IRQCHIP_EOI_THREADED)))