Re: [PATCHv2 4/5] irqchip/GICv3: let gic_handle_irq() utilize irqentry on arm64

[Mark Rutland <mark.rutland@arm.com>]

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)