Hi folks!
This is the spiritual successor to the below, which was over 6 years ago (!):
https://lore.kernel.org/lkml/[email protected]/
The series is available, along with my silly IRQ benchmark, at:
https://git.gitlab.arm.com/linux-arm/linux-vs.git -b mainline/irq/eoimodness-v3
Revisions
=========
RFCv2 -> v3
+++++++++++
o Rebased on top of tip/irq/core:
3d2ce675aba7 ("Merge tag 'irqchip-5.14' of git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms into irq/core")
o Tested with irqchip.gicv3_pseudo_nmi=1 using Marc's fixes
(arm64/for-next/cpuidle) on Ampere eMAG and Ampere Altra.
o Re-collected performance numbers for Juno and Ampere eMAG, also collected for
Ampere Altra
o Fixed s/irq_{ack, eoi}/{ack, eoi}_irq/ naming blunder (Marc)
o Gave msi_domain_update_chip_ops() default .irq_ack() and
.irq_eoi() (Marc)
Marc had suggested implementing a default callback that scans the domain
hierarchy for .irq_ack / .irq_eoi() and calls the first non-NULL
one. Now, things like nexus domains already have an irq_chip_eoi_parent();
leaving this would defeat using a "smarter" version in child domains, and
removing it felt like further obscuring the hierarchies. So just like
turtles, I went with irq_chip_{ack, eoi}_parent() all the way down.
o Added .irq_ack() callbacks to relevant GIC gadgets (Marc)
There might still be something to be done wrt chip flags, but I'll leave that
as it is for now. See my ramblings at:
http://lore.kernel.org/r/[email protected]
RFCv1 -> RFCv2
++++++++++++++
o Rebased against latest tip/irq/core
o Applied cleanups suggested by Thomas
o Collected some performance results
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.
Functional 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.
With Marc's pNMI vs cpuidle fixes (arm64/for-next/cpuidle), I also got to test
this against pNMIs on Ampere eMAG & Altra. Nothing to report here.
Performance impact
==================
Benchmark
+++++++++
Finding a benchmark that leverages a force-threaded IRQ has proved to be
somewhat of a pain, so I crafted my own. It's a bit daft, but so are most
benchmarks (though this one might win a prize).
Long story short, I'm picking an unused IRQ and have it be
force-threaded. The benchmark then is:
<bench thread>
loop:
irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, true);
wait_for_completion(&done);
<threaded handler>
complete(&done);
A more complete picture would be:
<bench thread> <whatever is on CPU0> <IRQ thread>
raise IRQ
wait
run flow handler
wake IRQ thread
finish handling
wake bench thread
Letting this run for a fixed amount of time lets me measure an entire IRQ
handling cycle, which is what I'm after since there's one less mask() in
the flow handler and one less unmask() in the threaded handler.
You'll note there's some potential "noise" in there due to scheduling both
the benchmark thread and the IRQ thread. However, the IRQ thread is pinned
to the IRQ's affinity, and I also pinned the benchmark thread in my tests,
which should keep this noise to a minimum.
Results
+++++++
20 iterations of 5 seconds of the above benchmark, measuring irqs/sec delta
between tip/irq/core and the series:
Juno r0:
| mean | median | 90th percentile | 99th percentile |
|------+--------+-----------------+-----------------|
| +6% | +6% | +6% | +6% |
Ampere eMAG:
| mean | median | 90th percentile | 99th percentile |
|------+--------+-----------------+-----------------|
| +21% | +22% | +20% | +20% |
Ampere Altra:
| mean | median | 90th percentile | 99th percentile |
|------+--------+-----------------+-----------------|
| +22% | +22% | +22% | +22% |
Cheers,
Valentin
Valentin Schneider (13):
genirq: Add chip flag to denote automatic IRQ (un)masking
genirq: Define ack_irq() and eoi_irq() 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
genirq/msi: Provide default .irq_eoi() for MSI chips
irqchip/gic: Rely on MSI default .irq_eoi()
genirq/msi: Provide default .irq_ack() for MSI chips
irqchip/gic: Add .irq_ack() to GIC-based irqchips
irqchip/gic: Convert to handle_strict_flow_irq()
irqchip/gic-v3: Convert to handle_strict_flow_irq()
drivers/base/platform-msi.c | 2 -
drivers/irqchip/irq-gic-v2m.c | 2 +-
drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 1 -
drivers/irqchip/irq-gic-v3-its-pci-msi.c | 1 -
drivers/irqchip/irq-gic-v3-its.c | 3 +
drivers/irqchip/irq-gic-v3-mbi.c | 2 +-
drivers/irqchip/irq-gic-v3.c | 27 +++--
drivers/irqchip/irq-gic.c | 14 ++-
include/linux/irq.h | 15 ++-
kernel/irq/chip.c | 122 +++++++++++++++++---
kernel/irq/debugfs.c | 2 +
kernel/irq/internals.h | 7 ++
kernel/irq/manage.c | 2 +-
kernel/irq/msi.c | 4 +
14 files changed, 166 insertions(+), 38 deletions(-)
--
2.25.1
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 <[email protected]>
---
kernel/irq/chip.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 793dbd8307b9..4d3bde55c5d9 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -429,10 +429,12 @@ static inline void mask_ack_irq(struct irq_desc *desc)
if (desc->irq_data.chip->irq_mask_ack) {
desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
irq_state_set_masked(desc);
+ if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
+ irq_state_set_flow_masked(desc);
} else {
mask_irq(desc);
if (desc->irq_data.chip->irq_ack)
- desc->irq_data.chip->irq_ack(&desc->irq_data);
+ ack_irq(desc);
}
}
@@ -463,7 +465,7 @@ void unmask_threaded_irq(struct irq_desc *desc)
struct irq_chip *chip = desc->irq_data.chip;
if (chip->flags & IRQCHIP_EOI_THREADED)
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
unmask_irq(desc);
}
@@ -680,7 +682,7 @@ EXPORT_SYMBOL_GPL(handle_level_irq);
static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
{
if (!(desc->istate & IRQS_ONESHOT)) {
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
return;
}
/*
@@ -691,10 +693,10 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
*/
if (!irqd_irq_disabled(&desc->irq_data) &&
irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
unmask_irq(desc);
} else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
}
}
--
2.25.1
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 <[email protected]>
---
kernel/irq/manage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index ef30b4762947..e6d6d32ddcbc 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1107,7 +1107,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc,
desc->threads_oneshot &= ~action->thread_mask;
if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
- irqd_irq_masked(&desc->irq_data))
+ (irqd_irq_masked(&desc->irq_data) | irqd_irq_flow_masked(&desc->irq_data)))
unmask_threaded_irq(desc);
out_unlock:
--
2.25.1
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 <[email protected]>
---
kernel/irq/chip.c | 16 ++++++++++++++++
kernel/irq/internals.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 21a21baa1366..793dbd8307b9 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -408,6 +408,22 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
cpumask_clear_cpu(cpu, desc->percpu_enabled);
}
+void ack_irq(struct irq_desc *desc)
+{
+ desc->irq_data.chip->irq_ack(&desc->irq_data);
+
+ if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
+ irq_state_set_flow_masked(desc);
+}
+
+void eoi_irq(struct irq_desc *desc)
+{
+ desc->irq_data.chip->irq_eoi(&desc->irq_data);
+
+ if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
+ irq_state_clr_flow_masked(desc);
+}
+
static inline void mask_ack_irq(struct irq_desc *desc)
{
if (desc->irq_data.chip->irq_mask_ack) {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index b6c1cceddec0..6d6a621dc74c 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -87,6 +87,8 @@ extern void irq_enable(struct irq_desc *desc);
extern void irq_disable(struct irq_desc *desc);
extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
+extern void ack_irq(struct irq_desc *desc);
+extern void eoi_irq(struct irq_desc *desc);
extern void mask_irq(struct irq_desc *desc);
extern void unmask_irq(struct irq_desc *desc);
extern void unmask_threaded_irq(struct irq_desc *desc);
--
2.25.1
On Tue, 29 Jun 2021 13:50:02 +0100,
Valentin Schneider <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> kernel/irq/manage.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index ef30b4762947..e6d6d32ddcbc 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1107,7 +1107,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc,
> desc->threads_oneshot &= ~action->thread_mask;
>
> if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
> - irqd_irq_masked(&desc->irq_data))
> + (irqd_irq_masked(&desc->irq_data) | irqd_irq_flow_masked(&desc->irq_data)))
> unmask_threaded_irq(desc);
The bitwise OR looks pretty odd. It is probably fine given that both
side of the expression are bool, but still. I can fix this locally.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Tue, 29 Jun 2021 13:49:59 +0100,
Valentin Schneider <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> kernel/irq/chip.c | 16 ++++++++++++++++
> kernel/irq/internals.h | 2 ++
> 2 files changed, 18 insertions(+)
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 21a21baa1366..793dbd8307b9 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -408,6 +408,22 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
> cpumask_clear_cpu(cpu, desc->percpu_enabled);
> }
>
> +void ack_irq(struct irq_desc *desc)
> +{
> + desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
> + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
> + irq_state_set_flow_masked(desc);
> +}
> +
> +void eoi_irq(struct irq_desc *desc)
> +{
> + desc->irq_data.chip->irq_eoi(&desc->irq_data);
> +
> + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
> + irq_state_clr_flow_masked(desc);
I just realised that this has a good chance to result in a mess with
KVM, and specially the way we let the vGIC deactivate an interrupt
directly from the guest, without any SW intervention (the magic HW bit
in the LRs).
With this, interrupts routed to a guest (such as the timers) will
always have the IRQD_IRQ_FLOW_MASKED flag set, which will never be
cleared.
I wonder whether this have a chance to interact badly with
mask/unmask, or with the rest of the flow...
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 12/08/21 08:49, Marc Zyngier wrote:
> On Tue, 29 Jun 2021 13:49:59 +0100,
> Valentin Schneider <[email protected]> wrote:
>> +void eoi_irq(struct irq_desc *desc)
>> +{
>> + desc->irq_data.chip->irq_eoi(&desc->irq_data);
>> +
>> + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
>> + irq_state_clr_flow_masked(desc);
>
> I just realised that this has a good chance to result in a mess with
> KVM, and specially the way we let the vGIC deactivate an interrupt
> directly from the guest, without any SW intervention (the magic HW bit
> in the LRs).
>
I didn't think to consider those. It can't ever be simple, can it...
> With this, interrupts routed to a guest (such as the timers) will
> always have the IRQD_IRQ_FLOW_MASKED flag set, which will never be
> cleared.
>
> I wonder whether this have a chance to interact badly with
> mask/unmask, or with the rest of the flow...
>
Isn't it the other way around? That is, eoi_irq() will clear
IRQD_IRQ_FLOW_MASKED regardless of what happens within chip->irq_eoi(),
so we would end up with !IRQD_IRQ_FLOW_MASKED even if the (physical) IRQ
remains Active (irqd_is_forwarded_to_vcpu() case).
This does not entirely match reality (if the IRQ is still Active then it is
still "flow-masked"), but AFAICT this doesn't impact our handling of
forwarded IRQs: IRQD_IRQ_FLOW_MASKED is only really relevant from ack_irq()
to eoi_irq(), and deactivation-from-the-guest (propagated via LR.HW=1)
happens after that.
On 12/08/21 08:26, Marc Zyngier wrote:
> On Tue, 29 Jun 2021 13:50:02 +0100,
> Valentin Schneider <[email protected]> wrote:
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index ef30b4762947..e6d6d32ddcbc 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -1107,7 +1107,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc,
>> desc->threads_oneshot &= ~action->thread_mask;
>>
>> if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>> - irqd_irq_masked(&desc->irq_data))
>> + (irqd_irq_masked(&desc->irq_data) | irqd_irq_flow_masked(&desc->irq_data)))
>> unmask_threaded_irq(desc);
>
> The bitwise OR looks pretty odd. It is probably fine given that both
> side of the expression are bool, but still. I can fix this locally.
>
Thomas suggested that back in v1:
https://lore.kernel.org/lkml/[email protected]/
I did look at the (arm64) disassembly diff back then and was convinced by
what I saw, though I'd have to go do that again as I can't remember much
else.
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On Thu, 12 Aug 2021 14:36:35 +0100,
Valentin Schneider <[email protected]> wrote:
>
> On 12/08/21 08:26, Marc Zyngier wrote:
> > On Tue, 29 Jun 2021 13:50:02 +0100,
> > Valentin Schneider <[email protected]> wrote:
> >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> >> index ef30b4762947..e6d6d32ddcbc 100644
> >> --- a/kernel/irq/manage.c
> >> +++ b/kernel/irq/manage.c
> >> @@ -1107,7 +1107,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc,
> >> desc->threads_oneshot &= ~action->thread_mask;
> >>
> >> if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
> >> - irqd_irq_masked(&desc->irq_data))
> >> + (irqd_irq_masked(&desc->irq_data) | irqd_irq_flow_masked(&desc->irq_data)))
> >> unmask_threaded_irq(desc);
> >
> > The bitwise OR looks pretty odd. It is probably fine given that both
> > side of the expression are bool, but still. I can fix this locally.
> >
>
> Thomas suggested that back in v1:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> I did look at the (arm64) disassembly diff back then and was convinced by
> what I saw, though I'd have to go do that again as I can't remember much
> else.
Ah, fair enough.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Thu, 12 Aug 2021 14:36:11 +0100,
Valentin Schneider <[email protected]> wrote:
>
> On 12/08/21 08:49, Marc Zyngier wrote:
> > On Tue, 29 Jun 2021 13:49:59 +0100,
> > Valentin Schneider <[email protected]> wrote:
> >> +void eoi_irq(struct irq_desc *desc)
> >> +{
> >> + desc->irq_data.chip->irq_eoi(&desc->irq_data);
> >> +
> >> + if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
> >> + irq_state_clr_flow_masked(desc);
> >
> > I just realised that this has a good chance to result in a mess with
> > KVM, and specially the way we let the vGIC deactivate an interrupt
> > directly from the guest, without any SW intervention (the magic HW bit
> > in the LRs).
> >
>
> I didn't think to consider those. It can't ever be simple, can it...
>
> > With this, interrupts routed to a guest (such as the timers) will
> > always have the IRQD_IRQ_FLOW_MASKED flag set, which will never be
> > cleared.
> >
> > I wonder whether this have a chance to interact badly with
> > mask/unmask, or with the rest of the flow...
> >
>
> Isn't it the other way around? That is, eoi_irq() will clear
> IRQD_IRQ_FLOW_MASKED regardless of what happens within chip->irq_eoi(),
> so we would end up with !IRQD_IRQ_FLOW_MASKED even if the (physical) IRQ
> remains Active (irqd_is_forwarded_to_vcpu() case).
Ah, I missed (again) that we always clear the flag, no matter what.
> This does not entirely match reality (if the IRQ is still Active then it is
> still "flow-masked"), but AFAICT this doesn't impact our handling of
> forwarded IRQs: IRQD_IRQ_FLOW_MASKED is only really relevant from ack_irq()
> to eoi_irq(), and deactivation-from-the-guest (propagated via LR.HW=1)
> happens after that.
Right. So we can have an active interrupt that is not flow-masked.
That's counter-intuitive, but that's the GIC architecture for you...
I'll take the series for a ride in -next. If anything breaks, we
should know pretty soon.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: 1b7a900c4da182de2022bee7cbf347d84291dda3
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/1b7a900c4da182de2022bee7cbf347d84291dda3
Author: Valentin Schneider <[email protected]>
AuthorDate: Tue, 29 Jun 2021 13:50:00 +01:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Thu, 12 Aug 2021 15:48:20 +01:00
genirq: Employ ack_irq() and eoi_irq() where relevant
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
kernel/irq/chip.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 793dbd8..4d3bde5 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -429,10 +429,12 @@ static inline void mask_ack_irq(struct irq_desc *desc)
if (desc->irq_data.chip->irq_mask_ack) {
desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
irq_state_set_masked(desc);
+ if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
+ irq_state_set_flow_masked(desc);
} else {
mask_irq(desc);
if (desc->irq_data.chip->irq_ack)
- desc->irq_data.chip->irq_ack(&desc->irq_data);
+ ack_irq(desc);
}
}
@@ -463,7 +465,7 @@ void unmask_threaded_irq(struct irq_desc *desc)
struct irq_chip *chip = desc->irq_data.chip;
if (chip->flags & IRQCHIP_EOI_THREADED)
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
unmask_irq(desc);
}
@@ -680,7 +682,7 @@ EXPORT_SYMBOL_GPL(handle_level_irq);
static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
{
if (!(desc->istate & IRQS_ONESHOT)) {
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
return;
}
/*
@@ -691,10 +693,10 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
*/
if (!irqd_irq_disabled(&desc->irq_data) &&
irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
unmask_irq(desc);
} else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
- chip->irq_eoi(&desc->irq_data);
+ eoi_irq(desc);
}
}
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: a4ea2933cc4581e70203a48c60bc26b69a936eeb
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/a4ea2933cc4581e70203a48c60bc26b69a936eeb
Author: Valentin Schneider <[email protected]>
AuthorDate: Tue, 29 Jun 2021 13:50:02 +01:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Thu, 12 Aug 2021 15:48:20 +01:00
genirq: Let purely flow-masked ONESHOT irqs through unmask_threaded_irq()
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
kernel/irq/manage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index ef30b47..e6d6d32 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1107,7 +1107,7 @@ again:
desc->threads_oneshot &= ~action->thread_mask;
if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
- irqd_irq_masked(&desc->irq_data))
+ (irqd_irq_masked(&desc->irq_data) | irqd_irq_flow_masked(&desc->irq_data)))
unmask_threaded_irq(desc);
out_unlock:
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: 635e4fd40660d189e1a83973ddd663abf7bbc849
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/635e4fd40660d189e1a83973ddd663abf7bbc849
Author: Valentin Schneider <[email protected]>
AuthorDate: Tue, 29 Jun 2021 13:49:59 +01:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Thu, 12 Aug 2021 15:48:19 +01:00
genirq: Define ack_irq() and eoi_irq() helpers
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
kernel/irq/chip.c | 16 ++++++++++++++++
kernel/irq/internals.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 21a21ba..793dbd8 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -408,6 +408,22 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
cpumask_clear_cpu(cpu, desc->percpu_enabled);
}
+void ack_irq(struct irq_desc *desc)
+{
+ desc->irq_data.chip->irq_ack(&desc->irq_data);
+
+ if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
+ irq_state_set_flow_masked(desc);
+}
+
+void eoi_irq(struct irq_desc *desc)
+{
+ desc->irq_data.chip->irq_eoi(&desc->irq_data);
+
+ if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
+ irq_state_clr_flow_masked(desc);
+}
+
static inline void mask_ack_irq(struct irq_desc *desc)
{
if (desc->irq_data.chip->irq_mask_ack) {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index b6c1cce..6d6a621 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -87,6 +87,8 @@ extern void irq_enable(struct irq_desc *desc);
extern void irq_disable(struct irq_desc *desc);
extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
+extern void ack_irq(struct irq_desc *desc);
+extern void eoi_irq(struct irq_desc *desc);
extern void mask_irq(struct irq_desc *desc);
extern void unmask_irq(struct irq_desc *desc);
extern void unmask_threaded_irq(struct irq_desc *desc);
On 12/08/21 15:45, Marc Zyngier wrote:
> On Thu, 12 Aug 2021 14:36:35 +0100,
> Valentin Schneider <[email protected]> wrote:
>>
>> On 12/08/21 08:26, Marc Zyngier wrote:
>> > On Tue, 29 Jun 2021 13:50:02 +0100,
>> > Valentin Schneider <[email protected]> wrote:
>> >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> >> index ef30b4762947..e6d6d32ddcbc 100644
>> >> --- a/kernel/irq/manage.c
>> >> +++ b/kernel/irq/manage.c
>> >> @@ -1107,7 +1107,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc,
>> >> desc->threads_oneshot &= ~action->thread_mask;
>> >>
>> >> if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>> >> - irqd_irq_masked(&desc->irq_data))
>> >> + (irqd_irq_masked(&desc->irq_data) | irqd_irq_flow_masked(&desc->irq_data)))
>> >> unmask_threaded_irq(desc);
>> >
>> > The bitwise OR looks pretty odd. It is probably fine given that both
>> > side of the expression are bool, but still. I can fix this locally.
>> >
>>
>> Thomas suggested that back in v1:
>>
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> I did look at the (arm64) disassembly diff back then and was convinced by
>> what I saw, though I'd have to go do that again as I can't remember much
>> else.
>
> Ah, fair enough.
>
Either I didn't have my glasses on or had a different output back then, but
I'm not so convinced anymore... (same result on both Ubuntu GCC 9.3.0 and
10.2 GCC release from Arm):
Logical OR:
8f8: b9400020 ldr w0, [x1]
8fc: 3787fea0 tbnz w0, #16, 8d0 <irq_finalize_oneshot.part.0+0x60>
900: 37880040 tbnz w0, #17, 908 <irq_finalize_oneshot.part.0+0x98>
904: 36fffe60 tbz w0, #31, 8d0 <irq_finalize_oneshot.part.0+0x60>
908: aa1303e0 mov x0, x19
90c: 94000000 bl 0 <unmask_threaded_irq>
Bitwise OR (aka the patch):
8f8: b9400020 ldr w0, [x1]
8fc: 3787fea0 tbnz w0, #16, 8d0 <irq_finalize_oneshot.part.0+0x60>
900: f26f001f tst x0, #0x20000
904: 7a400801 ccmp w0, #0x0, #0x1, eq // eq = none
908: 54fffe4a b.ge 8d0 <irq_finalize_oneshot.part.0+0x60> // b.tcont
90c: aa1303e0 mov x0, x19
910: 94000000 bl 0 <unmask_threaded_irq>
If I get this right...
- TST sets the Z condition flag if bit 17 (masked) isn't set
- CCMP sets the condition flags to
- the same as SUBS(flags, 0) if bit 17 wasn't set
- NZCV=0001 otherwise
- B.GE branches if N==V
Soooo
- if we have bit 17 set, NZCV=0001, B.GE doesn't branch
- if we don't have bit 17 but bit 31 (flow-masked), NZCV=1000 because
this is signed 32-bit, so having bit 31 set makes the result of
SUBS(flags, 0) negative, B.GE doesn't branch
- if we have neither, NZCV=0XX0, B.GE branches
So this does appear to do the right thing, at the cost of an extra
instruction and a profound sense of dread to whoever stares at the
disassembly. I guess it does save us a branch which could be
mispredicted...
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.