[GIT PULL] arm64 fixes for 5.15-rc5

[GIT PULL] arm64 fixes for 5.15-rc5

[Catalin Marinas]

Hi Linus,

Please pull the arm64 fixes below. Thanks.

The following changes since commit 22b70e6f2da0a4c8b1421b00cfc3016bc9d4d9d4:

  arm64: Restore forced disabling of KPTI on ThunderX (2021-09-23 15:59:15 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux tags/arm64-fixes

for you to fetch changes up to 0350419b14b98fd6f36801583360c36c8820c2e6:

  arm64/hugetlb: fix CMA gigantic page order for non-4K PAGE_SIZE (2021-10-06 11:11:13 +0100)

----------------------------------------------------------------
arm64 fixes:

- Avoid double accounting of irq_{entry,exit}_rcu().

- Fix CMA gigantic page order for 16K/64K page sizes.

----------------------------------------------------------------
Mark Rutland (1):
      arm64: entry: refactor EL1 interrupt entry logic

Mike Kravetz (1):
      arm64/hugetlb: fix CMA gigantic page order for non-4K PAGE_SIZE

Pingfan Liu (2):
      kernel/irq: make irq_{enter,exit}() in handle_domain_irq() arch optional
      arm64: entry: avoid double-accounting IRQ RCU entry

 arch/arm64/Kconfig               |  1 +
 arch/arm64/kernel/entry-common.c | 47 +++++++++++++++++++++-------------------
 arch/arm64/mm/hugetlbpage.c      |  2 +-
 kernel/irq/Kconfig               |  3 +++
 kernel/irq/irqdesc.c             |  4 ++++
 5 files changed, 34 insertions(+), 23 deletions(-)

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Linus Torvalds]

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.

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?

Instead it doubles down on that "let's do this non-arch-specific
accounting in arch-specific code", making the common code uglier and
weaker.

I initially pulled this, and then I just unpulled in disgust.

Please explain why arm64 does this bad thing, and why the fix isn't
"fix arm64", but instead "make the generic code uglier and harder to
maintain and follow".

Really, from all the explanations those commits give, the natural
thing to do would be "just fix arm64".

So if that really isn't the answer, then the explanations are clearly lacking.

                   Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Rutland]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Linus Torvalds]

On Mon, Oct 11, 2021 at 3:47 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> 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?

So I really hate that patch, even with explanation.

Having the Kconfig option for "do the right thing" is not how this
should be fixed.

Either the generic code is generic, or it isn't.

And if the generic code doesn't work for arm64, we shouldn't add a
random Kconfig option for "this architecture wants different
behavior".

> 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.

So then I think the fix is to just say "accounting in this generic
function is wrong, and the accounting needs to be moved to the
callers".

That's particularly true if you think arm64 is only the only actually
_tested_ case that gets this wrong, and other architectures most
likely have the exact same issue, but you only fixed it for arm64.

So do it unconditionally - possibly even using a coccinelle script if
there are lots of callers.

Because generic code that just isn't generic, but randomly does
different things based on subtle Kconfig options that are different
for different architectures is bad, bad, bad.

And yes, I realize that we've had that kind of stuff before (and we
have odd Kconfig option testing in that irqdesc.c file elsewhere), but
the Kconfig options we currently have are mostly either

 (a) actual real honest-to-goodness config options with semantic
meaning (ie things like CONFIG_SMP and CONFIG_NUMA)

 (b) really ugly workarounds for odd special module exports (that
CONFIG_KVM_BOOK3S_64_HV_MODULE thing for irq_to_desc() that we *tried*
but failed to remove).

And so the reason I really hate that patch is that it introduces a new
"different architectures randomly and inexplicably do different
things, and the generic behavior is very different on arm64 than it is
elsewhere".

That's just the worst kind of hack to me.

And in this case, it's really *horribly* hard to see what the call
chain is. It all ends up being actively obfuscated and obscured
through that 'handle_arch_irq' function pointer, that is sometimes set
through set_handle_irq(), and sometimes set directly.

I really think that if the rule is "we can't do accounting in
handle_domain_irq(), because it's too late for arm64", then the fix
really should be to just not do that.

Move the irq_enter()/irq_exit() to the callers - quite possibly far up
the call chain to the root of it all, and just say "architecture code
needs to do this in the low-level code before calling
handle_arch_irq".

Or, if it turns out that 99% of callers do want it - and don't have
the issues arm64 has - maybe we can have a helper wrapper that does
the irq_enter/irq_exit, and another version that doesn't do it, and at
least it's clear to the callers which one it is. But it looks like
particularly with the odd indirection through that handle_arch_irq
function, it's best to just say "this needs to be done".

What architectures actually care? From what I can follow, it's really
GENERIC_IRQ_MULTI_HANDLER that ends up doing this all, and then arm64
has it's own special case for no obvious reason (why isn't it using
GENERIC_IRQ_MULTI_HANDLER that seems to do the EXACT same thing in
generic code except for a special "default != NULL" case?)

Anyway, it _looks_ to me like the pattern is very simple:

Step 1:
 - remove irq_enter/irq_exit from handle_domain_irq(), move it to all callers.

This clearly doesn't change anything at all, but also doesn't fix the
problem you have. But it's easy to verify that the code is the same
before-and-after.

Step 2 is the pattern matching step:

 - if the caller of handle_domain_irq() ends up being a function that
is registered with set_handle_irq(), then we
   (a) remove the irq_enter/irq_exit from it
   (b) add it to the architectures that use handle_arch_irq.
   (c) make sure that if there are other callers of it (not through
handle_arch_irq) we move that irq_enter/irq_exit into them too

I _suspect_ - but didn't check - that Step 2(c) doesn't actually
exist. But who knows.

It really looks like there is a very tight connection between "uses
handle_domain_irq()" and "uses handle_arch_irq/set_handle_irq()". No?

Is this a bit more work? Yes.

Would this fix arm64? Yes - it ends up effectively doing what you did.

Would this fix _other_ architectures doing the same thing that you
suspect are broken? YES. Instead of leaving them randomly broken.

And most importantly, it would make the rules for "generic" code
clear, and actually generic.

Now, it may be that I'm wrong. I'm willing to be convinced, and if you
beat me over the head enough I guess I can take that pull request and
just be unhappy about it.

So I'm not trying to draw some line in the sand and be an arsehole and
say "you have to do it this way". But I look at that patch in that
pull request, and it really screams "this is horribly wrong" to me.

And I _think_ the above suggestion can be done almost mechanically.

And maybe it's easier to combine step1/step2 into one thing, if that
handle_domain_irq/handle_arch_irq" use is really as tightly bound as
it seems to be from a quick look.

So I'm not saying that they have to be separate steps, I wrote them
out that way mainly to kind of show the logic of the change - and in
case there are situations where you end up calling things without
going through that handle_arch_irq thing.

                       Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thomas Gleixner]

Linus,

On Mon, Oct 11 2021 at 12:54, Linus Torvalds wrote:
> On Mon, Oct 11, 2021 at 3:47 AM Mark Rutland <mark.rutland@arm.com> wrote:
> And so the reason I really hate that patch is that it introduces a new
> "different architectures randomly and inexplicably do different
> things, and the generic behavior is very different on arm64 than it is
> elsewhere".
>
> That's just the worst kind of hack to me.
>
> And in this case, it's really *horribly* hard to see what the call
> chain is. It all ends up being actively obfuscated and obscured
> through that 'handle_arch_irq' function pointer, that is sometimes set
> through set_handle_irq(), and sometimes set directly.
>
> I really think that if the rule is "we can't do accounting in
> handle_domain_irq(), because it's too late for arm64", then the fix
> really should be to just not do that.
>
> Move the irq_enter()/irq_exit() to the callers - quite possibly far up
> the call chain to the root of it all, and just say "architecture code
> needs to do this in the low-level code before calling
> handle_arch_irq".

That's where it belongs. It's mandatory to have it there for NOHZ_FULL
to work correctly vs. instrumentation etc. I've pointed that out back
then after we fed the X86 entry code into the mincer and added noinstr
sections to keep tracers, BPF and kprobes away from it.

Looking at the architectures which "support" that by selecting
HAVE_CONTEXT_TRACKING:

arch/arm/Kconfig:	select HAVE_CONTEXT_TRACKING
arch/arm64/Kconfig:	select HAVE_CONTEXT_TRACKING
arch/csky/Kconfig:	select HAVE_CONTEXT_TRACKING
arch/mips/Kconfig:	select HAVE_CONTEXT_TRACKING
arch/powerpc/Kconfig:	select HAVE_CONTEXT_TRACKING		if PPC64
arch/riscv/Kconfig:	select HAVE_CONTEXT_TRACKING
arch/sparc/Kconfig:	select HAVE_CONTEXT_TRACKING
arch/x86/Kconfig:	select HAVE_CONTEXT_TRACKING		if X86_64

S390 and X86 are (mostly) complete and use the generic entry code. S390
does not even select HAVE_CONTEXT_TRACKING!

PPC64 has done quite some work to fix that, but it looks not yet complete. 

Mark is working on ARM64.

There is some effort underway to convert MIPS over to generic entry.

The rest needs all the fundamental architecture side changes.

> Anyway, it _looks_ to me like the pattern is very simple:
>
> Step 1:
>  - remove irq_enter/irq_exit from handle_domain_irq(), move it to all callers.
>
> This clearly doesn't change anything at all, but also doesn't fix the
> problem you have. But it's easy to verify that the code is the same
> before-and-after.
>
> Step 2 is the pattern matching step:
>
>  - if the caller of handle_domain_irq() ends up being a function that
> is registered with set_handle_irq(), then we
>    (a) remove the irq_enter/irq_exit from it
>    (b) add it to the architectures that use handle_arch_irq.
>    (c) make sure that if there are other callers of it (not through
> handle_arch_irq) we move that irq_enter/irq_exit into them too
>
> I _suspect_ - but didn't check - that Step 2(c) doesn't actually
> exist. But who knows.

It only exists with chained handlers, but they do not need that at all
because:

        irq_enter()
        arch_handle_irq()
          handle_domain_irq()
            chained_handler()
              handle_domain_irq()

which is still the same interrupt context and not a nested interrupt.

> It really looks like there is a very tight connection between "uses
> handle_domain_irq()" and "uses handle_arch_irq/set_handle_irq()". No?

Looks like. That might conflict with the MIPS rework though. I don't
know how far that came already. Cc'ed the MIPS people.

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Rutland]

Hi Linus, Thomas,

A couple of minor comments below -- I'll have a go at the rework Linus
suggested and see what blows up.

On Tue, Oct 12, 2021 at 03:18:16PM +0200, Thomas Gleixner wrote:
> On Mon, Oct 11 2021 at 12:54, Linus Torvalds wrote:
> > On Mon, Oct 11, 2021 at 3:47 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > And so the reason I really hate that patch is that it introduces a new
> > "different architectures randomly and inexplicably do different
> > things, and the generic behavior is very different on arm64 than it is
> > elsewhere".
> >
> > That's just the worst kind of hack to me.
> >
> > And in this case, it's really *horribly* hard to see what the call
> > chain is. It all ends up being actively obfuscated and obscured
> > through that 'handle_arch_irq' function pointer, that is sometimes set
> > through set_handle_irq(), and sometimes set directly.
> >
> > I really think that if the rule is "we can't do accounting in
> > handle_domain_irq(), because it's too late for arm64", then the fix
> > really should be to just not do that.
> >
> > Move the irq_enter()/irq_exit() to the callers - quite possibly far up
> > the call chain to the root of it all, and just say "architecture code
> > needs to do this in the low-level code before calling
> > handle_arch_irq".
> 
> That's where it belongs. It's mandatory to have it there for NOHZ_FULL
> to work correctly vs. instrumentation etc. I've pointed that out back
> then after we fed the X86 entry code into the mincer and added noinstr
> sections to keep tracers, BPF and kprobes away from it.
> 
> Looking at the architectures which "support" that by selecting
> HAVE_CONTEXT_TRACKING:
> 
> arch/arm/Kconfig:	select HAVE_CONTEXT_TRACKING
> arch/arm64/Kconfig:	select HAVE_CONTEXT_TRACKING
> arch/csky/Kconfig:	select HAVE_CONTEXT_TRACKING
> arch/mips/Kconfig:	select HAVE_CONTEXT_TRACKING
> arch/powerpc/Kconfig:	select HAVE_CONTEXT_TRACKING		if PPC64
> arch/riscv/Kconfig:	select HAVE_CONTEXT_TRACKING
> arch/sparc/Kconfig:	select HAVE_CONTEXT_TRACKING
> arch/x86/Kconfig:	select HAVE_CONTEXT_TRACKING		if X86_64
> 
> S390 and X86 are (mostly) complete and use the generic entry code. S390
> does not even select HAVE_CONTEXT_TRACKING!
> 
> PPC64 has done quite some work to fix that, but it looks not yet complete. 
> 
> Mark is working on ARM64.
> 
> There is some effort underway to convert MIPS over to generic entry.
> 
> The rest needs all the fundamental architecture side changes.
> 
> > Anyway, it _looks_ to me like the pattern is very simple:
> >
> > Step 1:
> >  - remove irq_enter/irq_exit from handle_domain_irq(), move it to all callers.
> >
> > This clearly doesn't change anything at all, but also doesn't fix the
> > problem you have. But it's easy to verify that the code is the same
> > before-and-after.

I'm happy with this in principle. The only reason we didn't go down that
route initially is because the callers are (typically) in the bowels of
arch asm or platform code, they all need to be fixed in one go to avoid
breaking anything, and it's a headache if we collide with any rework
(e.g. MIPS moving to generic entry).

As above, I'll have a go and see what blows up.

It should be possible to have C wrappers for the common cases, and have
the asm call that instead of branching directly to whichever irqchip
handler handle_arch_irq points at.

> > Step 2 is the pattern matching step:
> >
> >  - if the caller of handle_domain_irq() ends up being a function that
> > is registered with set_handle_irq(), then we
> >    (a) remove the irq_enter/irq_exit from it
> >    (b) add it to the architectures that use handle_arch_irq.
> >    (c) make sure that if there are other callers of it (not through
> > handle_arch_irq) we move that irq_enter/irq_exit into them too
> >
> > I _suspect_ - but didn't check - that Step 2(c) doesn't actually
> > exist. But who knows.
> 
> It only exists with chained handlers, but they do not need that at all
> because:
> 
>         irq_enter()
>         arch_handle_irq()
>           handle_domain_irq()
>             chained_handler()
>               handle_domain_irq()

... and this is exactly the shape of case where we need to avoid the
nested calls so as to not break RCU's view of nesting.

> which is still the same interrupt context and not a nested interrupt.
> > It really looks like there is a very tight connection between "uses
> > handle_domain_irq()" and "uses handle_arch_irq/set_handle_irq()". No?
> 
> Looks like. That might conflict with the MIPS rework though. I don't
> know how far that came already. Cc'ed the MIPS people.

There's also a bunch of old platforms on arch/arm which have a
hard-coded handler (so not using handle_arch_irq/set_handle_irq()) which
calls handle_domain_irq() -- those can be fixed up.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thomas Gleixner]

On Tue, Oct 12 2021 at 15:02, Mark Rutland wrote:
> On Tue, Oct 12, 2021 at 03:18:16PM +0200, Thomas Gleixner wrote:
>> On Mon, Oct 11 2021 at 12:54, Linus Torvalds wrote:
> I'm happy with this in principle. The only reason we didn't go down that
> route initially is because the callers are (typically) in the bowels of
> arch asm or platform code, they all need to be fixed in one go to avoid
> breaking anything, and it's a headache if we collide with any rework
> (e.g. MIPS moving to generic entry).

mips-next looks pretty empty vs. that.

>> > It really looks like there is a very tight connection between "uses
>> > handle_domain_irq()" and "uses handle_arch_irq/set_handle_irq()". No?
>> 
>> Looks like. That might conflict with the MIPS rework though. I don't
>> know how far that came already. Cc'ed the MIPS people.
>
> There's also a bunch of old platforms on arch/arm which have a
> hard-coded handler (so not using handle_arch_irq/set_handle_irq()) which
> calls handle_domain_irq() -- those can be fixed up.

If that turns out to be ugly, then somehting like the below might be
less horrible as a stop gap.

Thanks,

        tglx
---

--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -268,20 +268,16 @@ void xen_send_IPI_allbutself(int vector)
 
 static irqreturn_t xen_call_function_interrupt(int irq, void *dev_id)
 {
-	irq_enter();
 	generic_smp_call_function_interrupt();
 	inc_irq_stat(irq_call_count);
-	irq_exit();
 
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t xen_call_function_single_interrupt(int irq, void *dev_id)
 {
-	irq_enter();
 	generic_smp_call_function_single_interrupt();
 	inc_irq_stat(irq_call_count);
-	irq_exit();
 
 	return IRQ_HANDLED;
 }
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -458,10 +458,8 @@ static void xen_pv_stop_other_cpus(int w
 
 static irqreturn_t xen_irq_work_interrupt(int irq, void *dev_id)
 {
-	irq_enter();
 	irq_work_run();
 	inc_irq_stat(apic_irq_work_irqs);
-	irq_exit();
 
 	return IRQ_HANDLED;
 }
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -33,6 +33,9 @@ config HOTPLUG_SMT
 config GENERIC_ENTRY
        bool
 
+config ARCH_ENTRY_RCU_CLEAN
+       bool
+
 config KPROBES
 	bool "Kprobes"
 	depends on MODULES
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -66,6 +66,7 @@ config X86
 	select ARCH_ENABLE_MEMORY_HOTREMOVE if MEMORY_HOTPLUG
 	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if (PGTABLE_LEVELS > 2) && (X86_64 || X86_PAE)
 	select ARCH_ENABLE_THP_MIGRATION if X86_64 && TRANSPARENT_HUGEPAGE
+	select ARCH_ENTRY_RCU_CLEAN
 	select ARCH_HAS_ACPI_TABLE_UPGRADE	if ACPI
 	select ARCH_HAS_CACHE_LINE_SIZE
 	select ARCH_HAS_DEBUG_VIRTUAL
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -677,24 +677,13 @@ int generic_handle_domain_irq(struct irq
 EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
 
 #ifdef CONFIG_HANDLE_DOMAIN_IRQ
-/**
- * handle_domain_irq - Invoke the handler for a HW irq belonging to a domain,
- *                     usually for a root interrupt controller
- * @domain:	The domain where to perform the lookup
- * @hwirq:	The HW irq number to convert to a logical one
- * @regs:	Register file coming from the low-level handling code
- *
- * Returns:	0 on success, or -EINVAL if conversion has failed
- */
-int handle_domain_irq(struct irq_domain *domain,
-		      unsigned int hwirq, struct pt_regs *regs)
+static int __handle_domain_irq(struct irq_domain *domain,
+			       unsigned int hwirq, struct pt_regs *regs)
 {
 	struct pt_regs *old_regs = set_irq_regs(regs);
 	struct irq_desc *desc;
 	int ret = 0;
 
-	irq_enter();
-
 	/* The irqdomain code provides boundary checks */
 	desc = irq_resolve_mapping(domain, hwirq);
 	if (likely(desc))
@@ -702,12 +691,41 @@ int handle_domain_irq(struct irq_domain
 	else
 		ret = -EINVAL;
 
-	irq_exit();
 	set_irq_regs(old_regs);
 	return ret;
 }
 
 /**
+ * handle_domain_irq - Invoke the handler for a HW irq belonging to a domain,
+ *                     usually for a root interrupt controller
+ * @domain:	The domain where to perform the lookup
+ * @hwirq:	The HW irq number to convert to a logical one
+ * @regs:	Register file coming from the low-level handling code
+ *
+ * Returns:	0 on success, or -EINVAL if conversion has failed
+ */
+#ifdef CONFIG_ARCH_ENTRY_RCU_CLEAN
+int handle_domain_irq(struct irq_domain *domain,
+		      unsigned int hwirq, struct pt_regs *regs)
+{
+	__handle_domain_irq(domain, hwirq, regs);
+}
+#else
+int handle_domain_irq(struct irq_domain *domain,
+		      unsigned int hwirq, struct pt_regs *regs)
+{
+	/*
+	 * irq_enter()/exit() has to be done in low level
+	 * architecture code. Bandaid for not yet fixed
+	 * architectures.
+	 */
+	irq_enter();
+	__handle_domain_irq(domain, hwirq, regs);
+	irq_exit();
+}
+#endif
+
+/**
  * handle_domain_nmi - Invoke the handler for a HW irq belonging to a domain
  * @domain:	The domain where to perform the lookup
  * @hwirq:	The HW irq number to convert to a logical one
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -601,6 +601,7 @@ void irq_enter_rcu(void)
 	account_hardirq_enter(current);
 }
 
+#ifndef ARCH_ENTRY_RCU_CLEAN
 /**
  * irq_enter - Enter an interrupt context including RCU update
  */
@@ -609,6 +610,7 @@ void irq_enter(void)
 	rcu_irq_enter();
 	irq_enter_rcu();
 }
+#endif
 
 static inline void tick_irq_exit(void)
 {
@@ -650,6 +652,7 @@ void irq_exit_rcu(void)
 	lockdep_hardirq_exit();
 }
 
+#ifndef ARCH_ENTRY_RCU_CLEAN
 /**
  * irq_exit - Exit an interrupt context, update RCU and lockdep
  *
@@ -662,6 +665,7 @@ void irq_exit(void)
 	 /* must be last! */
 	lockdep_hardirq_exit();
 }
+#endif
 
 /*
  * This function must run with irqs disabled!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Rutland]

On Tue, Oct 12, 2021 at 03:02:43PM +0100, Mark Rutland wrote:
> On Tue, Oct 12, 2021 at 03:18:16PM +0200, Thomas Gleixner wrote:
> > On Mon, Oct 11 2021 at 12:54, Linus Torvalds wrote:
> > > On Mon, Oct 11, 2021 at 3:47 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > > And so the reason I really hate that patch is that it introduces a new
> > > "different architectures randomly and inexplicably do different
> > > things, and the generic behavior is very different on arm64 than it is
> > > elsewhere".
> > >
> > > That's just the worst kind of hack to me.
> > >
> > > And in this case, it's really *horribly* hard to see what the call
> > > chain is. It all ends up being actively obfuscated and obscured
> > > through that 'handle_arch_irq' function pointer, that is sometimes set
> > > through set_handle_irq(), and sometimes set directly.
> > >
> > > I really think that if the rule is "we can't do accounting in
> > > handle_domain_irq(), because it's too late for arm64", then the fix
> > > really should be to just not do that.
> > >
> > > Move the irq_enter()/irq_exit() to the callers - quite possibly far up
> > > the call chain to the root of it all, and just say "architecture code
> > > needs to do this in the low-level code before calling
> > > handle_arch_irq".

I've spent the last few days attacking this, and I have a series which
reworks things to pull irq_{enter,exit}() out of the irqchip code and
into arch/entry code where it belongs, removig CONFIG_HANDLE_DOMAIN_IRQ
entirely in the process. I'll post that out soon once I've cleaned up
the commit messages and given it a decent cover letter.

> > > Anyway, it _looks_ to me like the pattern is very simple:
> > >
> > > Step 1:
> > >  - remove irq_enter/irq_exit from handle_domain_irq(), move it to all callers.
> > >
> > > This clearly doesn't change anything at all, but also doesn't fix the
> > > problem you have. But it's easy to verify that the code is the same
> > > before-and-after.
> > >
> > > Step 2 is the pattern matching step:
> > >
> > >  - if the caller of handle_domain_irq() ends up being a function that
> > > is registered with set_handle_irq(), then we
> > >    (a) remove the irq_enter/irq_exit from it
> > >    (b) add it to the architectures that use handle_arch_irq.
> > >    (c) make sure that if there are other callers of it (not through
> > > handle_arch_irq) we move that irq_enter/irq_exit into them too
> > >
> > > I _suspect_ - but didn't check - that Step 2(c) doesn't actually

I had a go with the approach suggested above, but that didn't really
work out and I ended up splitting the problem a different way. Comments
belwo for the sake of posterity.

Attacking this as a per-caller issue is *really* chury, and
interdependencies force you to fix all drivers and all architectures in
one go, which makes it really hard to see the wood for the trees.

The underlying issue was with CONFIG_HANDLE_DOMAIN_IRQ, so just looking
as set_handle_irq (which indicates CONFIG_GENERIC_IRQ_MULTI_HANDLER)
also wasn't sufficient, and I had to go digging through each of the
affected architectures' entry code.

Instead, I've added a temporary shim, migrated each architecture in
turn, then removed the shim and CONFIG_HANDLE_DOMAIN_IRQ entirely, which
also ends up simplifying the drivers a bit.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Pingfan Liu]

Hi Linus,

I am not in the Cc list, so just aware of it the day before yesterday.

On Mon, Oct 11, 2021 at 12:54:49PM -0700, Linus Torvalds wrote:
> On Mon, Oct 11, 2021 at 3:47 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > 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?
> 
> So I really hate that patch, even with explanation.
> 
> Having the Kconfig option for "do the right thing" is not how this
> should be fixed.
> 
> Either the generic code is generic, or it isn't.
> 
> And if the generic code doesn't work for arm64, we shouldn't add a
> random Kconfig option for "this architecture wants different
> behavior".
> 
> > 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.
> 
> So then I think the fix is to just say "accounting in this generic
> function is wrong, and the accounting needs to be moved to the
> callers".
> 
> That's particularly true if you think arm64 is only the only actually
> _tested_ case that gets this wrong, and other architectures most
> likely have the exact same issue, but you only fixed it for arm64.
> 
> So do it unconditionally - possibly even using a coccinelle script if
> there are lots of callers.
> 
> Because generic code that just isn't generic, but randomly does
> different things based on subtle Kconfig options that are different
> for different architectures is bad, bad, bad.
> 

When composing the patch, I failed to realize it, but now, I learn.

> And yes, I realize that we've had that kind of stuff before (and we
> have odd Kconfig option testing in that irqdesc.c file elsewhere), but
> the Kconfig options we currently have are mostly either
> 
>  (a) actual real honest-to-goodness config options with semantic
> meaning (ie things like CONFIG_SMP and CONFIG_NUMA)
> 
>  (b) really ugly workarounds for odd special module exports (that
> CONFIG_KVM_BOOK3S_64_HV_MODULE thing for irq_to_desc() that we *tried*
> but failed to remove).
> 
> And so the reason I really hate that patch is that it introduces a new
> "different architectures randomly and inexplicably do different
> things, and the generic behavior is very different on arm64 than it is
> elsewhere".
> 
> That's just the worst kind of hack to me.
> 
> And in this case, it's really *horribly* hard to see what the call
> chain is. It all ends up being actively obfuscated and obscured
> through that 'handle_arch_irq' function pointer, that is sometimes set
> through set_handle_irq(), and sometimes set directly.
> 
> I really think that if the rule is "we can't do accounting in
> handle_domain_irq(), because it's too late for arm64", then the fix
> really should be to just not do that.
> 
> Move the irq_enter()/irq_exit() to the callers - quite possibly far up
> the call chain to the root of it all, and just say "architecture code
> needs to do this in the low-level code before calling
> handle_arch_irq".
> 
> Or, if it turns out that 99% of callers do want it - and don't have
> the issues arm64 has - maybe we can have a helper wrapper that does
> the irq_enter/irq_exit, and another version that doesn't do it, and at
> least it's clear to the callers which one it is. But it looks like
> particularly with the odd indirection through that handle_arch_irq
> function, it's best to just say "this needs to be done".
> 
> What architectures actually care? From what I can follow, it's really
> GENERIC_IRQ_MULTI_HANDLER that ends up doing this all, and then arm64
> has it's own special case for no obvious reason (why isn't it using
> GENERIC_IRQ_MULTI_HANDLER that seems to do the EXACT same thing in
> generic code except for a special "default != NULL" case?)
> 
> Anyway, it _looks_ to me like the pattern is very simple:
> 
> Step 1:
>  - remove irq_enter/irq_exit from handle_domain_irq(), move it to all callers.
> 
> This clearly doesn't change anything at all, but also doesn't fix the
> problem you have. But it's easy to verify that the code is the same
> before-and-after.
> 
> Step 2 is the pattern matching step:
> 
>  - if the caller of handle_domain_irq() ends up being a function that
> is registered with set_handle_irq(), then we
>    (a) remove the irq_enter/irq_exit from it
>    (b) add it to the architectures that use handle_arch_irq.
>    (c) make sure that if there are other callers of it (not through
> handle_arch_irq) we move that irq_enter/irq_exit into them too
> 
> I _suspect_ - but didn't check - that Step 2(c) doesn't actually
> exist. But who knows.
> 
> It really looks like there is a very tight connection between "uses
> handle_domain_irq()" and "uses handle_arch_irq/set_handle_irq()". No?
> 
> Is this a bit more work? Yes.
> 
> Would this fix arm64? Yes - it ends up effectively doing what you did.
> 
> Would this fix _other_ architectures doing the same thing that you
> suspect are broken? YES. Instead of leaving them randomly broken.
> 
> And most importantly, it would make the rules for "generic" code
> clear, and actually generic.
> 
> Now, it may be that I'm wrong. I'm willing to be convinced, and if you
> beat me over the head enough I guess I can take that pull request and
> just be unhappy about it.
> 

I had thought if any arch adapts GENERIC_ENTRY, then handle_domain_irq()
can be a bug. As GENERIC_ENTRY is a not _random_ config option, so try
to re-anchor the config depending on GENERIC_ENTRY.

But finally I gave up, since there is no direct link between them at a
glance. And what is more, as you said, "the rules for "generic" code
clear, and actually generic", so it is better to go in that way.

I think Mark has started the work or I will be happy to re-work on these
patches.

Thanks,

	Pingfan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel