Re: [GIT PULL] arm64 fixes for 5.15-rc5

Mark Rutland
2021-10-11T10:47:37Z

Hi Linus,

[adding Paul and Peter, just in case we need to discuss the RCU and
accounting bits further]

On Fri, Oct 08, 2021 at 01:25:51PM -0700, Linus Torvalds wrote:
> On Fri, Oct 8, 2021 at 11:37 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> >
> > Pingfan Liu (2):
> > kernel/irq: make irq_{enter,exit}() in handle_domain_irq() arch optional
> > arm64: entry: avoid double-accounting IRQ RCU entry
>
> Ugh. This is *really* ugly. And it seems to be going exactly the wrong way.
>
> I read the commit descriptions, and it still doesn't answer the
> fundamental question of why arm64 needs to do the accounting in
> arch-specific code, and disable the generic code.
>
> It says
>
> 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.
>
> but at no point does it actually explain *why* all the accounting
> needs to be done by the architecture code.

Sorry; I agree that commit messages don't explain this thoroughly. I can
go and rework the commit messages to clarify things, if you'd like?

The TL;DR is that a bunch of constraints conspire such that we can't
defer accounting things to the irqdomain or irqchip code without making
this even more complicated/fragile, and many architectures get this
subtly wrong today -- it's not that arm64 is necessarily much different
from other architectures using this code, just that we're trying to
solve this first.

More specifically, the problem here is the combination of:

* During entry, rcu_irq_enter() must be called *before*
trace_hardirqs_off_finish(), because the latter needs RCU to be
watching, but we need to have informed lockdep before poking RCU or
lockdep will complain withing the RCU code. To handle that,
kernel/entry/common.c and arch/arm64/kernel/entry-common.c have the
sequence:

lockdep_hardirqs_off(CALLER_ADDR0);
rcu_irq_enter(): // or rcu_irq_enter_check_tick();
trace_hardirqs_off_finish();

... and since we don't want something to come in the middle of the
sequence, we want those sandwiched together in a noinstr function
called before we invoke the root irqchip's handler function.

A bunch of architectures don't follow this sequence, and are
potentially subtly broken today in some configurations.

Note: the plan is to move arm64 over to the generic entry code, but
that has no effect on this specific issue.

* rcu_irq_enter() must be called *precisely once* upon taking an
interrupt exception, or we can end up mis-accounting quiescent periods
as non-quiescent (since this maintains a nesting count in
rcu_data::dynticks_nmi_nesting, checked by
rcu_is_cpu_rrupt_from_idle()).

* Prior to these patches, we call rcu_irq_enter() at least twice on
arm64, because the architectural entry code must call rcu_irq_enter()
for the lockdep bits, then when we invoke the irqchip (e.g. GICv3), we
do:

gic_handle_irq()
handle_domain_irq()
irq_enter()
rcu_irq_enter()
irq_enter_rcu()

... and so if we take a sched clock IRQ and call
rcu_sched_clock_irq(), rcu_is_cpu_rrupt_from_idle() may return false
when it should return true(), and we don't decide to preempt the task,
leading to stalls.

Note that since irqchips can be chained (e.g. there can be a second
interrupt controller fed into the GIC), handle_domain_irq() can be
nested, so even if we somehow removed the accounting from arch code
we'd need to handle the nested calls to handle_domain_irq() somehow.

AFAICT it's far simpler to do that *once* outside of the irqchip code
than it is to try to handle that nesting.

Note that x86 doesn't use handle_domain_irq(), and just maps the raw
irqnrs itself, and just calls irq_enter_rcu() when transitioning to
the IRQ stack.

> Yes, yes, I read the previous paragraph. But why isn't the fix to just
> stop doing the double accounting in the arm64 specific code?

As above, that'd require moving *some* of the low-level entry logic into
the irqchip/irqdomain code, and handling nesting, which is *more*
fragile.

FWIW, we do need to fix the other architectures too, but that involves
more substantial rework to each of those (e.g. moving to generic entry),
since there are a bunch of problems today. The thinking was that this
gave a way to fix each of those on its own, then remove the old
behaviour.

Does that all make sense to you?

Thanks,
Mark.