At present, the irq entry/exit accounting, which is performed by
handle_domain_irq(), overlaps with arm64 exception entry code somehow.
By supplementing irq accounting on arm64 exception entry code, the
accounting in handle_domain_irq() can be dropped totally by selecting
the macro HAVE_ARCH_IRQENTRY.
Signed-off-by: Pingfan Liu <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Joey Gouly <[email protected]>
Cc: Sami Tolvanen <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Yuichi Ito <[email protected]>
Cc: [email protected]
To: [email protected]
---
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/entry-common.c | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5c7ae4c3954b..d29bae38a951 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -98,6 +98,7 @@ config ARM64
select ARCH_HAS_UBSAN_SANITIZE_ALL
select ARM_AMBA
select ARM_ARCH_TIMER
+ select HAVE_ARCH_IRQENTRY
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
select ARM_GIC_V2M if PCI
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 5f1473319fb0..6d4dc3b3799f 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -428,7 +428,9 @@ static __always_inline void
__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
{
enter_from_kernel_mode(regs);
+ irq_enter_rcu();
do_interrupt_handler(regs, handler);
+ irq_exit_rcu();
/*
* Note: thread_info::preempt_count includes both thread_info::count
* and thread_info::need_resched, and is not equivalent to
@@ -667,7 +669,9 @@ static void noinstr el0_interrupt(struct pt_regs *regs,
if (regs->pc & BIT(55))
arm64_apply_bp_hardening();
+ irq_enter_rcu();
do_interrupt_handler(regs, handler);
+ irq_exit_rcu();
exit_to_user_mode(regs);
}
--
2.31.1
On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> At present, the irq entry/exit accounting, which is performed by
> handle_domain_irq(), overlaps with arm64 exception entry code somehow.
>
> By supplementing irq accounting on arm64 exception entry code, the
> accounting in handle_domain_irq() can be dropped totally by selecting
> the macro HAVE_ARCH_IRQENTRY.
I think we need to be more thorough and explain the specific problem and
solution. How about we crib some wording from patch 1, and say:
arm64: entry: avoid double-accounting IRQ RCU entry
When an IRQ is taken, some accounting needs to be performed to enter
and exit IRQ context around the IRQ handler. On arm64 some of this
accounting is performed by both the architecture code and the IRQ
domain code, resulting in calling rcu_irq_enter() twice per exception
entry, violating the expectations of the core RCU code, and resulting
in failing to identify quiescent periods correctly (e.g. in
rcu_is_cpu_rrupt_from_idle()).
To fix this, we must perform all the accounting from the architecture
code. We prevent the IRQ domain code from performing any accounting by
selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
irq_exit_rcu() around invoking the root IRQ handler.
When we take a pNMI from a context with IRQs disabled, we'll perform
the necessary accounting as part of arm64_enter_nmi() and
arm64_exit_nmi(), and should only call irq_enter_rcu() and
irq_exit_rcu() when we may have taken a regular interrupt.
That way it's clear what specifically the overlap is and the problem(s)
it results in. The bit at the end explains why we don't call
irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.
> Signed-off-by: Pingfan Liu <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Joey Gouly <[email protected]>
> Cc: Sami Tolvanen <[email protected]>
> Cc: Julien Thierry <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Yuichi Ito <[email protected]>
> Cc: [email protected]
> To: [email protected]
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/entry-common.c | 4 ++++
> 2 files changed, 5 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5c7ae4c3954b..d29bae38a951 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -98,6 +98,7 @@ config ARM64
> select ARCH_HAS_UBSAN_SANITIZE_ALL
> select ARM_AMBA
> select ARM_ARCH_TIMER
> + select HAVE_ARCH_IRQENTRY
Please put this with the other HAVE_ARCH_* entries in
arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.
With that and the title and commit message above:
Reviewed-by: Mark Rutland <[email protected]>
Thanks,
Mark.
> select ARM_GIC
> select AUDIT_ARCH_COMPAT_GENERIC
> select ARM_GIC_V2M if PCI
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 5f1473319fb0..6d4dc3b3799f 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -428,7 +428,9 @@ static __always_inline void
> __el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> {
> enter_from_kernel_mode(regs);
> + irq_enter_rcu();
> do_interrupt_handler(regs, handler);
> + irq_exit_rcu();
> /*
> * Note: thread_info::preempt_count includes both thread_info::count
> * and thread_info::need_resched, and is not equivalent to
> @@ -667,7 +669,9 @@ static void noinstr el0_interrupt(struct pt_regs *regs,
> if (regs->pc & BIT(55))
> arm64_apply_bp_hardening();
>
> + irq_enter_rcu();
> do_interrupt_handler(regs, handler);
> + irq_exit_rcu();
>
> exit_to_user_mode(regs);
> }
> --
> 2.31.1
>
On Thu, 30 Sep 2021 14:53:14 +0100,
Mark Rutland <[email protected]> wrote:
>
> On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> > At present, the irq entry/exit accounting, which is performed by
> > handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> >
> > By supplementing irq accounting on arm64 exception entry code, the
> > accounting in handle_domain_irq() can be dropped totally by selecting
> > the macro HAVE_ARCH_IRQENTRY.
>
> I think we need to be more thorough and explain the specific problem and
> solution. How about we crib some wording from patch 1, and say:
>
> arm64: entry: avoid double-accounting IRQ RCU entry
>
> When an IRQ is taken, some accounting needs to be performed to enter
> and exit IRQ context around the IRQ handler. On arm64 some of this
> accounting is performed by both the architecture code and the IRQ
> domain code, resulting in calling rcu_irq_enter() twice per exception
> entry, violating the expectations of the core RCU code, and resulting
> in failing to identify quiescent periods correctly (e.g. in
> rcu_is_cpu_rrupt_from_idle()).
>
> To fix this, we must perform all the accounting from the architecture
> code. We prevent the IRQ domain code from performing any accounting by
> selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
> irq_exit_rcu() around invoking the root IRQ handler.
>
> When we take a pNMI from a context with IRQs disabled, we'll perform
> the necessary accounting as part of arm64_enter_nmi() and
> arm64_exit_nmi(), and should only call irq_enter_rcu() and
> irq_exit_rcu() when we may have taken a regular interrupt.
>
> That way it's clear what specifically the overlap is and the problem(s)
> it results in. The bit at the end explains why we don't call
> irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.
>
> > Signed-off-by: Pingfan Liu <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Joey Gouly <[email protected]>
> > Cc: Sami Tolvanen <[email protected]>
> > Cc: Julien Thierry <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Yuichi Ito <[email protected]>
> > Cc: [email protected]
> > To: [email protected]
> > ---
> > arch/arm64/Kconfig | 1 +
> > arch/arm64/kernel/entry-common.c | 4 ++++
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..d29bae38a951 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -98,6 +98,7 @@ config ARM64
> > select ARCH_HAS_UBSAN_SANITIZE_ALL
> > select ARM_AMBA
> > select ARM_ARCH_TIMER
> > + select HAVE_ARCH_IRQENTRY
>
> Please put this with the other HAVE_ARCH_* entries in
> arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
> HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.
>
> With that and the title and commit message above:
>
> Reviewed-by: Mark Rutland <[email protected]>
With the above changes,
Reviewed-by: Marc Zyngier <[email protected]>
M.
--
Without deviation from the norm, progress is not possible.
On Thu, Sep 30, 2021 at 02:53:14PM +0100, Mark Rutland wrote:
> On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> > At present, the irq entry/exit accounting, which is performed by
> > handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> >
> > By supplementing irq accounting on arm64 exception entry code, the
> > accounting in handle_domain_irq() can be dropped totally by selecting
> > the macro HAVE_ARCH_IRQENTRY.
>
> I think we need to be more thorough and explain the specific problem and
> solution. How about we crib some wording from patch 1, and say:
>
> arm64: entry: avoid double-accounting IRQ RCU entry
>
> When an IRQ is taken, some accounting needs to be performed to enter
> and exit IRQ context around the IRQ handler. On arm64 some of this
> accounting is performed by both the architecture code and the IRQ
> domain code, resulting in calling rcu_irq_enter() twice per exception
> entry, violating the expectations of the core RCU code, and resulting
> in failing to identify quiescent periods correctly (e.g. in
> rcu_is_cpu_rrupt_from_idle()).
>
> To fix this, we must perform all the accounting from the architecture
> code. We prevent the IRQ domain code from performing any accounting by
> selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
> irq_exit_rcu() around invoking the root IRQ handler.
>
> When we take a pNMI from a context with IRQs disabled, we'll perform
> the necessary accounting as part of arm64_enter_nmi() and
> arm64_exit_nmi(), and should only call irq_enter_rcu() and
> irq_exit_rcu() when we may have taken a regular interrupt.
>
It is a wonderful and elaborated log.
> That way it's clear what specifically the overlap is and the problem(s)
> it results in. The bit at the end explains why we don't call
> irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.
>
I have learned much from the log you contribute to this series. I will keep
learning how to improve my ability of log. Thanks again!
> > Signed-off-by: Pingfan Liu <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Joey Gouly <[email protected]>
> > Cc: Sami Tolvanen <[email protected]>
> > Cc: Julien Thierry <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Yuichi Ito <[email protected]>
> > Cc: [email protected]
> > To: [email protected]
> > ---
> > arch/arm64/Kconfig | 1 +
> > arch/arm64/kernel/entry-common.c | 4 ++++
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..d29bae38a951 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -98,6 +98,7 @@ config ARM64
> > select ARCH_HAS_UBSAN_SANITIZE_ALL
> > select ARM_AMBA
> > select ARM_ARCH_TIMER
> > + select HAVE_ARCH_IRQENTRY
>
> Please put this with the other HAVE_ARCH_* entries in
> arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
> HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.
>
OK, I will fix it in V4.
> With that and the title and commit message above:
>
> Reviewed-by: Mark Rutland <[email protected]>
>
Thanks,
Pingfan