| Message ID | 20210924132837.45994-5-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/irqentry: remove duplicate housekeeping of | expand |
On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > The call to rcu_irq_enter() originated from gic_handle_irq() is > redundant now, since arm64 has enter_from_kernel_mode() akin to > irqenter_entry(), which has already called rcu_irq_enter(). Here I think you're referring to the call in handle_domain_irq(), but that isn't clear from the commit message. > Based on code analysis, the redundant can raise some mistake, e.g. > rcu_data->dynticks_nmi_nesting inc 2, which causes > rcu_is_cpu_rrupt_from_idle() unexpected. > > So eliminate the call to irq_enter() in handle_domain_irq(). And > accordingly supplementing irq_enter_rcu(). We support many more irqchips on arm64, and GICv3 can be used on regular 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call into the GICv3 driver specifically breaks other drivers on arm64 by removing the call, and breaks the GICv3 driver on arm by adding a duplicate call. It looks like this should live in do_interrupt_handler() in arch/arm64/kernel/entry-common.c, e.g. | static void do_interrupt_handler(struct pt_regs *regs, | void (*handler)(struct pt_regs *)) | { | irq_enter_rcu(); | if (on_thread_stack()) | call_on_irq_stack(regs, handler); | else | handler(regs); | irq_exit_rcu(); | } ... unless there's some problem with that? Thanks, Mark. > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Joey Gouly <joey.gouly@arm.com> > Cc: Sami Tolvanen <samitolvanen@google.com> > Cc: Julien Thierry <julien.thierry@arm.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com> > Cc: linux-kernel@vger.kernel.org > To: linux-arm-kernel@lists.infradead.org > --- > arch/arm64/Kconfig | 1 + > drivers/irqchip/irq-gic-v3.c | 2 ++ > 2 files changed, 3 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/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 89dcec902a82..906538fa8771 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -729,10 +729,12 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs > else > isb(); > > + irq_enter_rcu(); > if (handle_domain_irq(gic_data.domain, irqnr, regs)) { > WARN_ONCE(true, "Unexpected interrupt received!\n"); > gic_deactivate_unhandled(irqnr); > } > + irq_exit_rcu(); > } > > static u32 gic_get_pribits(void) > -- > 2.31.1 >
On Tue, Sep 28, 2021 at 10:10:53AM +0100, Mark Rutland wrote: > On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > > The call to rcu_irq_enter() originated from gic_handle_irq() is > > redundant now, since arm64 has enter_from_kernel_mode() akin to > > irqenter_entry(), which has already called rcu_irq_enter(). > > Here I think you're referring to the call in handle_domain_irq(), but > that isn't clear from the commit message. > Yes, and I will make it clear in V2. > > Based on code analysis, the redundant can raise some mistake, e.g. > > rcu_data->dynticks_nmi_nesting inc 2, which causes > > rcu_is_cpu_rrupt_from_idle() unexpected. > > > > So eliminate the call to irq_enter() in handle_domain_irq(). And > > accordingly supplementing irq_enter_rcu(). > > We support many more irqchips on arm64, and GICv3 can be used on regular > 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call > into the GICv3 driver specifically breaks other drivers on arm64 by > removing the call, and breaks the GICv3 driver on arm by adding a > duplicate call. > Oops. I forgot to protect the code in GICv3 with CONFIG_HAVE_ARCH_IRQENTRY > It looks like this should live in do_interrupt_handler() in > arch/arm64/kernel/entry-common.c, e.g. > > | static void do_interrupt_handler(struct pt_regs *regs, > | void (*handler)(struct pt_regs *)) > | { > | irq_enter_rcu(); > | if (on_thread_stack()) > | call_on_irq_stack(regs, handler); > | else > | handler(regs); > | irq_exit_rcu(); > | } > > ... unless there's some problem with that? > Yeah, do_interrupt_handler() is a more suitable place. But to resolve the performance regression of rescheduling IPI [1], it is badly demanded to distinguish irqnr before calling irq_enter_rcu() (please see 5/5 and [2] for the context). So it is a compromise to host the code in GICv3. Any good idea? [1]: https://lore.kernel.org/linux-arm-kernel/20201101131430.257038-1-maz@kernel.org/ [2]: https://lore.kernel.org/linux-arm-kernel/87lfewnmdz.fsf@nanos.tec.linutronix.de/ Thanks, Pingfan
On Wed, 29 Sep 2021 04:10:11 +0100, Pingfan Liu <piliu@redhat.com> wrote: > > On Tue, Sep 28, 2021 at 10:10:53AM +0100, Mark Rutland wrote: > > On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > > > The call to rcu_irq_enter() originated from gic_handle_irq() is > > > redundant now, since arm64 has enter_from_kernel_mode() akin to > > > irqenter_entry(), which has already called rcu_irq_enter(). > > > > Here I think you're referring to the call in handle_domain_irq(), but > > that isn't clear from the commit message. > > > Yes, and I will make it clear in V2. > > > > Based on code analysis, the redundant can raise some mistake, e.g. > > > rcu_data->dynticks_nmi_nesting inc 2, which causes > > > rcu_is_cpu_rrupt_from_idle() unexpected. > > > > > > So eliminate the call to irq_enter() in handle_domain_irq(). And > > > accordingly supplementing irq_enter_rcu(). > > > > We support many more irqchips on arm64, and GICv3 can be used on regular > > 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call > > into the GICv3 driver specifically breaks other drivers on arm64 by > > removing the call, and breaks the GICv3 driver on arm by adding a > > duplicate call. > > > Oops. I forgot to protect the code in GICv3 with CONFIG_HAVE_ARCH_IRQENTRY > > > It looks like this should live in do_interrupt_handler() in > > arch/arm64/kernel/entry-common.c, e.g. > > > > | static void do_interrupt_handler(struct pt_regs *regs, > > | void (*handler)(struct pt_regs *)) > > | { > > | irq_enter_rcu(); > > | if (on_thread_stack()) > > | call_on_irq_stack(regs, handler); > > | else > > | handler(regs); > > | irq_exit_rcu(); > > | } > > > > ... unless there's some problem with that? > > > Yeah, do_interrupt_handler() is a more suitable place. But to resolve > the performance regression of rescheduling IPI [1], it is badly demanded to > distinguish irqnr before calling irq_enter_rcu() (please see 5/5 and [2] > for the context). So it is a compromise to host the code in GICv3. > > Any good idea? There is no way we are going to single out a particular interrupt controller. As for the "regression", we'll have to look at the numbers once we have fixed the whole infrastructure. M.
On Wed, Sep 29, 2021 at 08:20:35AM +0100, Marc Zyngier wrote: > On Wed, 29 Sep 2021 04:10:11 +0100, > Pingfan Liu <piliu@redhat.com> wrote: > > > > On Tue, Sep 28, 2021 at 10:10:53AM +0100, Mark Rutland wrote: > > > On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > > > > The call to rcu_irq_enter() originated from gic_handle_irq() is > > > > redundant now, since arm64 has enter_from_kernel_mode() akin to > > > > irqenter_entry(), which has already called rcu_irq_enter(). > > > > > > Here I think you're referring to the call in handle_domain_irq(), but > > > that isn't clear from the commit message. > > > > > Yes, and I will make it clear in V2. > > > > > > Based on code analysis, the redundant can raise some mistake, e.g. > > > > rcu_data->dynticks_nmi_nesting inc 2, which causes > > > > rcu_is_cpu_rrupt_from_idle() unexpected. > > > > > > > > So eliminate the call to irq_enter() in handle_domain_irq(). And > > > > accordingly supplementing irq_enter_rcu(). > > > > > > We support many more irqchips on arm64, and GICv3 can be used on regular > > > 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call > > > into the GICv3 driver specifically breaks other drivers on arm64 by > > > removing the call, and breaks the GICv3 driver on arm by adding a > > > duplicate call. > > > > > Oops. I forgot to protect the code in GICv3 with CONFIG_HAVE_ARCH_IRQENTRY > > > > > It looks like this should live in do_interrupt_handler() in > > > arch/arm64/kernel/entry-common.c, e.g. > > > > > > | static void do_interrupt_handler(struct pt_regs *regs, > > > | void (*handler)(struct pt_regs *)) > > > | { > > > | irq_enter_rcu(); > > > | if (on_thread_stack()) > > > | call_on_irq_stack(regs, handler); > > > | else > > > | handler(regs); > > > | irq_exit_rcu(); > > > | } > > > > > > ... unless there's some problem with that? > > > > > Yeah, do_interrupt_handler() is a more suitable place. But to resolve > > the performance regression of rescheduling IPI [1], it is badly demanded to > > distinguish irqnr before calling irq_enter_rcu() (please see 5/5 and [2] > > for the context). So it is a compromise to host the code in GICv3. > > > > Any good idea? > > There is no way we are going to single out a particular interrupt > controller. As for the "regression", we'll have to look at the numbers > once we have fixed the whole infrastructure. > But I just realize that at present, gic_handle_nmi() sits behind gic_handle_irq(). So it will make an mistaken for accounting of normal interrupt if calling irq_enter_rcu() in do_interrupt_handler(). And going through drivers/irqchip/irq-chip-gic*, I think there are only two files should be handled: irq-gic.c and irq-gic-v3.c. Thanks, Pingfan
On Wed, Sep 29, 2021 at 04:27:11PM +0800, Pingfan Liu wrote: > On Wed, Sep 29, 2021 at 08:20:35AM +0100, Marc Zyngier wrote: > > On Wed, 29 Sep 2021 04:10:11 +0100, > > Pingfan Liu <piliu@redhat.com> wrote: > > > > > > On Tue, Sep 28, 2021 at 10:10:53AM +0100, Mark Rutland wrote: > > > > On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > > > > > The call to rcu_irq_enter() originated from gic_handle_irq() is > > > > > redundant now, since arm64 has enter_from_kernel_mode() akin to > > > > > irqenter_entry(), which has already called rcu_irq_enter(). > > > > > > > > Here I think you're referring to the call in handle_domain_irq(), but > > > > that isn't clear from the commit message. > > > > > > > Yes, and I will make it clear in V2. > > > > > > > > Based on code analysis, the redundant can raise some mistake, e.g. > > > > > rcu_data->dynticks_nmi_nesting inc 2, which causes > > > > > rcu_is_cpu_rrupt_from_idle() unexpected. > > > > > > > > > > So eliminate the call to irq_enter() in handle_domain_irq(). And > > > > > accordingly supplementing irq_enter_rcu(). > > > > > > > > We support many more irqchips on arm64, and GICv3 can be used on regular > > > > 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call > > > > into the GICv3 driver specifically breaks other drivers on arm64 by > > > > removing the call, and breaks the GICv3 driver on arm by adding a > > > > duplicate call. > > > > > > > Oops. I forgot to protect the code in GICv3 with CONFIG_HAVE_ARCH_IRQENTRY > > > > > > > It looks like this should live in do_interrupt_handler() in > > > > arch/arm64/kernel/entry-common.c, e.g. > > > > > > > > | static void do_interrupt_handler(struct pt_regs *regs, > > > > | void (*handler)(struct pt_regs *)) > > > > | { > > > > | irq_enter_rcu(); > > > > | if (on_thread_stack()) > > > > | call_on_irq_stack(regs, handler); > > > > | else > > > > | handler(regs); > > > > | irq_exit_rcu(); > > > > | } > > > > > > > > ... unless there's some problem with that? > > > > > > > Yeah, do_interrupt_handler() is a more suitable place. But to resolve > > > the performance regression of rescheduling IPI [1], it is badly demanded to > > > distinguish irqnr before calling irq_enter_rcu() (please see 5/5 and [2] > > > for the context). So it is a compromise to host the code in GICv3. > > > > > > Any good idea? > > > > There is no way we are going to single out a particular interrupt > > controller. As for the "regression", we'll have to look at the numbers > > once we have fixed the whole infrastructure. > > > But I just realize that at present, gic_handle_nmi() sits behind > gic_handle_irq(). So it will make an mistaken for accounting of normal > interrupt if calling irq_enter_rcu() in do_interrupt_handler(). We can restructure entry-common.c to avoid that if necessary. TBH, the more I see problems in this area the more I want to rip out the pNMI bits... > And going through drivers/irqchip/irq-chip-gic*, I think there are only > two files should be handled: irq-gic.c and irq-gic-v3.c. That are irqchips other than GICv2 and GICv3 that are used as the root irqchip on arm64. For example, Raspberry Pi 3 uses drivers/irqchip/irq-bcm2836.c as its root irqchip. Thanks, Mark.
On Wed, Sep 29, 2021 at 10:23:58AM +0100, Mark Rutland wrote: > On Wed, Sep 29, 2021 at 04:27:11PM +0800, Pingfan Liu wrote: > > On Wed, Sep 29, 2021 at 08:20:35AM +0100, Marc Zyngier wrote: > > > On Wed, 29 Sep 2021 04:10:11 +0100, > > > Pingfan Liu <piliu@redhat.com> wrote: > > > > > > > > On Tue, Sep 28, 2021 at 10:10:53AM +0100, Mark Rutland wrote: > > > > > On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > > > > > > The call to rcu_irq_enter() originated from gic_handle_irq() is > > > > > > redundant now, since arm64 has enter_from_kernel_mode() akin to > > > > > > irqenter_entry(), which has already called rcu_irq_enter(). > > > > > > > > > > Here I think you're referring to the call in handle_domain_irq(), but > > > > > that isn't clear from the commit message. > > > > > > > > > Yes, and I will make it clear in V2. > > > > > > > > > > Based on code analysis, the redundant can raise some mistake, e.g. > > > > > > rcu_data->dynticks_nmi_nesting inc 2, which causes > > > > > > rcu_is_cpu_rrupt_from_idle() unexpected. > > > > > > > > > > > > So eliminate the call to irq_enter() in handle_domain_irq(). And > > > > > > accordingly supplementing irq_enter_rcu(). > > > > > > > > > > We support many more irqchips on arm64, and GICv3 can be used on regular > > > > > 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call > > > > > into the GICv3 driver specifically breaks other drivers on arm64 by > > > > > removing the call, and breaks the GICv3 driver on arm by adding a > > > > > duplicate call. > > > > > > > > > Oops. I forgot to protect the code in GICv3 with CONFIG_HAVE_ARCH_IRQENTRY > > > > > > > > > It looks like this should live in do_interrupt_handler() in > > > > > arch/arm64/kernel/entry-common.c, e.g. > > > > > > > > > > | static void do_interrupt_handler(struct pt_regs *regs, > > > > > | void (*handler)(struct pt_regs *)) > > > > > | { > > > > > | irq_enter_rcu(); > > > > > | if (on_thread_stack()) > > > > > | call_on_irq_stack(regs, handler); > > > > > | else > > > > > | handler(regs); > > > > > | irq_exit_rcu(); > > > > > | } > > > > > > > > > > ... unless there's some problem with that? > > > > > > > > > Yeah, do_interrupt_handler() is a more suitable place. But to resolve > > > > the performance regression of rescheduling IPI [1], it is badly demanded to > > > > distinguish irqnr before calling irq_enter_rcu() (please see 5/5 and [2] > > > > for the context). So it is a compromise to host the code in GICv3. > > > > > > > > Any good idea? > > > > > > There is no way we are going to single out a particular interrupt > > > controller. As for the "regression", we'll have to look at the numbers > > > once we have fixed the whole infrastructure. > > > > > But I just realize that at present, gic_handle_nmi() sits behind > > gic_handle_irq(). So it will make an mistaken for accounting of normal > > interrupt if calling irq_enter_rcu() in do_interrupt_handler(). > > We can restructure entry-common.c to avoid that if necessary. > > TBH, the more I see problems in this area the more I want to rip out the > pNMI bits... > Could you give further comments and some guide to my reply to [1/5], which can help to decide pNMI at this earlier stage? If things can go that way, then everything can be fixed easier. I think they abstract the ability of irqchip by exporting two interfaces: void (*handle_arch_nmi)(struct pt_regs *regs); bool (*interrupt_is_nmi)(void); And each irqchip can select whether to implement or not. > > And going through drivers/irqchip/irq-chip-gic*, I think there are only > > two files should be handled: irq-gic.c and irq-gic-v3.c. > > That are irqchips other than GICv2 and GICv3 that are used as the root > irqchip on arm64. For example, Raspberry Pi 3 uses > drivers/irqchip/irq-bcm2836.c as its root irqchip. > Thanks for the explanation. The situation is worse than I had thought. And no way out in that direction. Thanks, Pingfan
On Wed, Sep 29, 2021 at 10:23:58AM +0100, Mark Rutland wrote: > On Wed, Sep 29, 2021 at 04:27:11PM +0800, Pingfan Liu wrote: > > On Wed, Sep 29, 2021 at 08:20:35AM +0100, Marc Zyngier wrote: > > > On Wed, 29 Sep 2021 04:10:11 +0100, > > > Pingfan Liu <piliu@redhat.com> wrote: > > > > > > > > On Tue, Sep 28, 2021 at 10:10:53AM +0100, Mark Rutland wrote: > > > > > On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > > > > > > The call to rcu_irq_enter() originated from gic_handle_irq() is > > > > > > redundant now, since arm64 has enter_from_kernel_mode() akin to > > > > > > irqenter_entry(), which has already called rcu_irq_enter(). > > > > > > > > > > Here I think you're referring to the call in handle_domain_irq(), but > > > > > that isn't clear from the commit message. > > > > > > > > > Yes, and I will make it clear in V2. > > > > > > > > > > Based on code analysis, the redundant can raise some mistake, e.g. > > > > > > rcu_data->dynticks_nmi_nesting inc 2, which causes > > > > > > rcu_is_cpu_rrupt_from_idle() unexpected. > > > > > > > > > > > > So eliminate the call to irq_enter() in handle_domain_irq(). And > > > > > > accordingly supplementing irq_enter_rcu(). > > > > > > > > > > We support many more irqchips on arm64, and GICv3 can be used on regular > > > > > 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call > > > > > into the GICv3 driver specifically breaks other drivers on arm64 by > > > > > removing the call, and breaks the GICv3 driver on arm by adding a > > > > > duplicate call. > > > > > > > > > Oops. I forgot to protect the code in GICv3 with CONFIG_HAVE_ARCH_IRQENTRY > > > > > > > > > It looks like this should live in do_interrupt_handler() in > > > > > arch/arm64/kernel/entry-common.c, e.g. > > > > > > > > > > | static void do_interrupt_handler(struct pt_regs *regs, > > > > > | void (*handler)(struct pt_regs *)) > > > > > | { > > > > > | irq_enter_rcu(); > > > > > | if (on_thread_stack()) > > > > > | call_on_irq_stack(regs, handler); > > > > > | else > > > > > | handler(regs); > > > > > | irq_exit_rcu(); > > > > > | } > > > > > > > > > > ... unless there's some problem with that? > > > > > > > > > Yeah, do_interrupt_handler() is a more suitable place. But to resolve > > > > the performance regression of rescheduling IPI [1], it is badly demanded to > > > > distinguish irqnr before calling irq_enter_rcu() (please see 5/5 and [2] > > > > for the context). So it is a compromise to host the code in GICv3. > > > > > > > > Any good idea? > > > > > > There is no way we are going to single out a particular interrupt > > > controller. As for the "regression", we'll have to look at the numbers > > > once we have fixed the whole infrastructure. > > > > > But I just realize that at present, gic_handle_nmi() sits behind > > gic_handle_irq(). So it will make an mistaken for accounting of normal > > interrupt if calling irq_enter_rcu() in do_interrupt_handler(). > > We can restructure entry-common.c to avoid that if necessary. > > TBH, the more I see problems in this area the more I want to rip out the > pNMI bits... > Overlook the undetermined pNMI, what about the partial patch like the following: diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c index 32f9796c4ffe..3c46f8fd0e2e 100644 --- a/arch/arm64/kernel/entry-common.c +++ b/arch/arm64/kernel/entry-common.c @@ -219,17 +219,20 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs) lockdep_hardirqs_on(CALLER_ADDR0); } -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs) +static bool noinstr enter_el1_irq_or_nmi(struct pt_regs *regs) { - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) + if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) { arm64_enter_nmi(regs); - else + return false; + } else { enter_from_kernel_mode(regs); + return true; + } } -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs) +static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs, bool is_irq) { - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) + if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !is_irq) arm64_exit_nmi(regs); else exit_to_kernel_mode(regs); @@ -261,12 +264,19 @@ static void __sched arm64_preempt_schedule_irq(void) } static void do_interrupt_handler(struct pt_regs *regs, - void (*handler)(struct pt_regs *)) + void (*handler)(struct pt_regs *), + bool is_irq) { + if (likely(is_irq)) + irq_enter_rcu(); + if (on_thread_stack()) call_on_irq_stack(regs, handler); else handler(regs); + + if (likely(is_irq)) + irq_exit_rcu(); } extern void (*handle_arch_irq)(struct pt_regs *); @@ -435,10 +445,12 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs) static void noinstr el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *)) { + bool is_irq; + write_sysreg(DAIF_PROCCTX_NOIRQ, daif); - enter_el1_irq_or_nmi(regs); - do_interrupt_handler(regs, handler); + is_irq = enter_el1_irq_or_nmi(regs); + do_interrupt_handler(regs, handler, is_irq); /* * Note: thread_info::preempt_count includes both thread_info::count @@ -449,7 +461,7 @@ static void noinstr el1_interrupt(struct pt_regs *regs, READ_ONCE(current_thread_info()->preempt_count) == 0) arm64_preempt_schedule_irq(); - exit_el1_irq_or_nmi(regs); + exit_el1_irq_or_nmi(regs, is_irq); } asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
On Wed, Sep 29, 2021 at 10:29:09PM +0800, Pingfan Liu wrote: > On Wed, Sep 29, 2021 at 10:23:58AM +0100, Mark Rutland wrote: > > On Wed, Sep 29, 2021 at 04:27:11PM +0800, Pingfan Liu wrote: > > > On Wed, Sep 29, 2021 at 08:20:35AM +0100, Marc Zyngier wrote: > > > > On Wed, 29 Sep 2021 04:10:11 +0100, > > > > Pingfan Liu <piliu@redhat.com> wrote: > > > > > > > > > > On Tue, Sep 28, 2021 at 10:10:53AM +0100, Mark Rutland wrote: > > > > > > On Fri, Sep 24, 2021 at 09:28:36PM +0800, Pingfan Liu wrote: > > > > > > > The call to rcu_irq_enter() originated from gic_handle_irq() is > > > > > > > redundant now, since arm64 has enter_from_kernel_mode() akin to > > > > > > > irqenter_entry(), which has already called rcu_irq_enter(). > > > > > > > > > > > > Here I think you're referring to the call in handle_domain_irq(), but > > > > > > that isn't clear from the commit message. > > > > > > > > > > > Yes, and I will make it clear in V2. > > > > > > > > > > > > Based on code analysis, the redundant can raise some mistake, e.g. > > > > > > > rcu_data->dynticks_nmi_nesting inc 2, which causes > > > > > > > rcu_is_cpu_rrupt_from_idle() unexpected. > > > > > > > > > > > > > > So eliminate the call to irq_enter() in handle_domain_irq(). And > > > > > > > accordingly supplementing irq_enter_rcu(). > > > > > > > > > > > > We support many more irqchips on arm64, and GICv3 can be used on regular > > > > > > 32-bit arm, so this isn't right. Moving the irq_enter_rcu() call > > > > > > into the GICv3 driver specifically breaks other drivers on arm64 by > > > > > > removing the call, and breaks the GICv3 driver on arm by adding a > > > > > > duplicate call. > > > > > > > > > > > Oops. I forgot to protect the code in GICv3 with CONFIG_HAVE_ARCH_IRQENTRY > > > > > > > > > > > It looks like this should live in do_interrupt_handler() in > > > > > > arch/arm64/kernel/entry-common.c, e.g. > > > > > > > > > > > > | static void do_interrupt_handler(struct pt_regs *regs, > > > > > > | void (*handler)(struct pt_regs *)) > > > > > > | { > > > > > > | irq_enter_rcu(); > > > > > > | if (on_thread_stack()) > > > > > > | call_on_irq_stack(regs, handler); > > > > > > | else > > > > > > | handler(regs); > > > > > > | irq_exit_rcu(); > > > > > > | } > > > > > > > > > > > > ... unless there's some problem with that? > > > > > > > > > > > Yeah, do_interrupt_handler() is a more suitable place. But to resolve > > > > > the performance regression of rescheduling IPI [1], it is badly demanded to > > > > > distinguish irqnr before calling irq_enter_rcu() (please see 5/5 and [2] > > > > > for the context). So it is a compromise to host the code in GICv3. > > > > > > > > > > Any good idea? > > > > > > > > There is no way we are going to single out a particular interrupt > > > > controller. As for the "regression", we'll have to look at the numbers > > > > once we have fixed the whole infrastructure. > > > > > > > But I just realize that at present, gic_handle_nmi() sits behind > > > gic_handle_irq(). So it will make an mistaken for accounting of normal > > > interrupt if calling irq_enter_rcu() in do_interrupt_handler(). > > > > We can restructure entry-common.c to avoid that if necessary. > > > > TBH, the more I see problems in this area the more I want to rip out the > > pNMI bits... > > > > Overlook the undetermined pNMI, what about the partial patch like the following: > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > index 32f9796c4ffe..3c46f8fd0e2e 100644 > --- a/arch/arm64/kernel/entry-common.c > +++ b/arch/arm64/kernel/entry-common.c > @@ -219,17 +219,20 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs) > lockdep_hardirqs_on(CALLER_ADDR0); > } > > -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs) > +static bool noinstr enter_el1_irq_or_nmi(struct pt_regs *regs) > { > - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > + if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) { > arm64_enter_nmi(regs); > - else > + return false; > + } else { > enter_from_kernel_mode(regs); > + return true; > + } > } > > -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs) > +static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs, bool is_irq) > { > - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > + if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !is_irq) > arm64_exit_nmi(regs); > else > exit_to_kernel_mode(regs); > @@ -261,12 +264,19 @@ static void __sched arm64_preempt_schedule_irq(void) > } > > static void do_interrupt_handler(struct pt_regs *regs, > - void (*handler)(struct pt_regs *)) > + void (*handler)(struct pt_regs *), > + bool is_irq) > { > + if (likely(is_irq)) > + irq_enter_rcu(); > + > if (on_thread_stack()) > call_on_irq_stack(regs, handler); > else > handler(regs); > + > + if (likely(is_irq)) > + irq_exit_rcu(); > } I'm not keen on having a bunch of conditional calls like this, since it's easy to get wrong and static analyzers are liable to complain if they don't accurately track the stat of the condition variable across multiple blocks, and tbh I wasn't too keen on the *_irq_or_nmi() helpers after I wrote them. I reckon structurally the below would be better; we can add the irq_{enter,exit}_rcu() calls in __el1_interrupt() and el0_interrupt(), around the calls to do_interrupt_handler(), and it makes it clearer by that we won't preempt if IRQs were masked in the interrupted context (which the preempt_count manipulation in __nmi_{enter,exit} ensures today). Thanks, Mark. ---->8---- diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c index 32f9796c4ffe..7af7ddbea4b6 100644 --- a/arch/arm64/kernel/entry-common.c +++ b/arch/arm64/kernel/entry-common.c @@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs) lockdep_hardirqs_on(CALLER_ADDR0); } -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs) -{ - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) - arm64_enter_nmi(regs); - else - enter_from_kernel_mode(regs); -} - -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs) -{ - if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) - arm64_exit_nmi(regs); - else - exit_to_kernel_mode(regs); -} - static void __sched arm64_preempt_schedule_irq(void) { lockdep_assert_irqs_disabled(); @@ -432,12 +416,18 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs) } } -static void noinstr el1_interrupt(struct pt_regs *regs, - void (*handler)(struct pt_regs *)) +static __always_inline void +__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *)) { - write_sysreg(DAIF_PROCCTX_NOIRQ, daif); + arm64_enter_nmi(regs); + do_interrupt_handler(regs, handler); + arm64_exit_nmi(regs); +} - enter_el1_irq_or_nmi(regs); +static __always_inline void +__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *)) +{ + enter_from_kernel_mode(regs); do_interrupt_handler(regs, handler); /* @@ -449,7 +439,18 @@ static void noinstr el1_interrupt(struct pt_regs *regs, READ_ONCE(current_thread_info()->preempt_count) == 0) arm64_preempt_schedule_irq(); - exit_el1_irq_or_nmi(regs); + exit_to_kernel_mode(regs); +} + +static void noinstr el1_interrupt(struct pt_regs *regs, + void (*handler)(struct pt_regs *)) +{ + write_sysreg(DAIF_PROCCTX_NOIRQ, daif); + + if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) + __el1_pnmi(regs, handler); + else + __el1_interrupt(regs, handler); } asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
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/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 89dcec902a82..906538fa8771 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -729,10 +729,12 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs else isb(); + irq_enter_rcu(); if (handle_domain_irq(gic_data.domain, irqnr, regs)) { WARN_ONCE(true, "Unexpected interrupt received!\n"); gic_deactivate_unhandled(irqnr); } + irq_exit_rcu(); } static u32 gic_get_pribits(void)
The call to rcu_irq_enter() originated from gic_handle_irq() is redundant now, since arm64 has enter_from_kernel_mode() akin to irqenter_entry(), which has already called rcu_irq_enter(). Based on code analysis, the redundant can raise some mistake, e.g. rcu_data->dynticks_nmi_nesting inc 2, which causes rcu_is_cpu_rrupt_from_idle() unexpected. So eliminate the call to irq_enter() in handle_domain_irq(). And accordingly supplementing irq_enter_rcu(). Signed-off-by: Pingfan Liu <kernelfans@gmail.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Joey Gouly <joey.gouly@arm.com> Cc: Sami Tolvanen <samitolvanen@google.com> Cc: Julien Thierry <julien.thierry@arm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Yuichi Ito <ito-yuichi@fujitsu.com> Cc: linux-kernel@vger.kernel.org To: linux-arm-kernel@lists.infradead.org --- arch/arm64/Kconfig | 1 + drivers/irqchip/irq-gic-v3.c | 2 ++ 2 files changed, 3 insertions(+)