Re: [PATCHv2 0/5] arm64/irqentry: remove duplicate housekeeping of

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

On Tue, Sep 28, 2021 at 11:52:51AM +0200, Sven Schnelle wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
> 
> > On Mon, Sep 27, 2021 at 05:09:22PM -0700, Paul E. McKenney wrote:
> >> On Mon, Sep 27, 2021 at 10:23:18AM +0100, Mark Rutland wrote:
> >> > Sure; I didn't mean to suggest those weren't balanced! The problem here
> >> > is *nesting*. Due to the structure of our entry code and the core IRQ
> >> > code, when handling an IRQ we have a sequence:
> >> > 
> >> > 	irq_enter() // arch code
> >> > 	irq_enter() // irq code
> >> > 
> >> > 	< irq handler here >
> >> > 
> >> > 	irq_exit() // irq code
> >> > 	irq_exit() // arch code
> >> > 
> >> > ... and if we use something like rcu_is_cpu_rrupt_from_idle() in the
> >> > middle (e.g. as part of rcu_sched_clock_irq()), this will not give the
> >> > expected result because of the additional nesting, since
> >> > rcu_is_cpu_rrupt_from_idle() seems to expect that dynticks_nmi_nesting
> >> > is only incremented once per exception entry, when it does:
> >> > 
> >> > 	/* Are we at first interrupt nesting level? */
> >> > 	nesting = __this_cpu_read(rcu_data.dynticks_nmi_nesting);
> >> > 	if (nesting > 1)
> >> > 		return false;
> >> > 
> >> > What I'm trying to figure out is whether that expectation is legitimate,
> >> > and assuming so, where the entry/exit should happen.
> >> 
> >> Oooh...
> >> 
> >> The penalty for fooling rcu_is_cpu_rrupt_from_idle() is that RCU will
> >> be unable to detect a userspace quiescent state for a non-nohz_full
> >> CPU.  That could result in RCU CPU stall warnings if a user task runs
> >> continuously on a given CPU for more than 21 seconds (60 seconds in
> >> some distros).  And this can easily happen if the user has a CPU-bound
> >> thread that is the only runnable task on that CPU.
> >> 
> >> So, yes, this does need some sort of resolution.
> >> 
> >> The traditional approach is (as you surmise) to have only a single call
> >> to irq_enter() on exception entry and only a single call to irq_exit()
> >> on exception exit.  If this is feasible, it is highly recommended.
> >
> > Cool; that's roughly what I was expecting / hoping to hear!
> >
> >> In theory, we could have that "1" in "nesting > 1" be a constant supplied
> >> by the architecture (you would want "3" if I remember correctly) but
> >> in practice could we please avoid this?  For one thing, if there is
> >> some other path into the kernel for your architecture that does only a
> >> single irq_enter(), then rcu_is_cpu_rrupt_from_idle() just doesn't stand
> >> a chance.  It would need to compare against a different value depending
> >> on what exception showed up.  Even if that cannot happen, it would be
> >> better if your architecture could remain in blissful ignorance of the
> >> colorful details of ->dynticks_nmi_nesting manipulations.
> >
> > I completely agree. I think it's much harder to keep that in check than
> > to enforce a "once per architectural exception" policy in the arch code.
> >
> >> Another approach would be for the arch code to supply RCU a function that
> >> it calls.  If there is such a function (or perhaps better, if some new
> >> Kconfig option is enabled), RCU invokes it.  Otherwise, it compares to
> >> "1" as it does now.  But you break it, you buy it!  ;-)
> >
> > I guess we could look at the exception regs and inspect the original
> > context, but it sounds overkill...
> >
> > I think the cleanest thing is to leave this to arch code, and have the
> > common IRQ code stay well clear. Unfortunately most architectures
> > (including arch/arm) still need the common IRQ code to handle this, so
> > we'll have to make that conditional on Kconfig, something like the below
> > (build+boot tested only).
> >
> > If there are no objections, I'll go check who else needs the same
> > treatment (IIUC at least s390 will), and spin that as a real
> > patch/series.
> 
> Hmm, s390 doesn't use handle_domain_irq() and doesn't have
> HANDLE_DOMAIN_IRQ set. So i don't think the patch below applies to s390.
> However, i'll follow the code to make sure we're not calling
> irq_enter/irq_exit twice.

I wasn't clear, but for s390, my concern was that in do_io_irq() and
do_ext_irq() you have the sequence:

	irqentry_enter()	// calls rcu_irq_enter()
	irq_enter();		// calls rcu_irq_enter() then irq_enter_rcu();

	< handler>

	irq_exit();		// calls __irq_exit_rcu then rcu_irq_exit();
	irqentry_exit();	// calls rcu_irq_exit()

... and so IIUC you call rcu_irq_enter() and rcu_irq_exit() twice,
getting the same double-increment of `dynticks_nmi_nesting` per
interrupt, and the same potential problem with
rcu_is_cpu_rrupt_from_idle().

Thanks,
Mark.