Message-ID: <CAHk-=wgZSD3W2y6yczad2Am=EfHYyiPzTn3CfXxrriJf9i5W5w@mail.gmail.com>
Date:   Fri, 5 Aug 2022 10:29:53 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Arnd Bergmann <arnd@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-arch <linux-arch@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [GIT PULL] asm-generic: updates for 6.0

On Fri, Aug 5, 2022 at 3:25 AM Arnd Bergmann <arnd@...nel.org> wrote:
>
>  - A patch from Sebastian Siewior to rework the handling of
>    IRQ stacks in softirqs across architectures, which is
>    needed for enabling PREEMPT_RT.

I am quite fed up with the chaos and garbage that PREEMPT_RT has
caused this release.

Once again, this pull request contains senseless code because of a
PREEMPT_RT patch that was merged based on some bogus "the RT code
needs this".

First off, the RT code isn't currently enabled in upstream kernels, so
none of this merits any kind of big hurry and mindless "need to apply
because it's a bug".

Secondly, that patch is HORRENDOUSLY UGLY.

I hereby ask every single maintainer to immediately stop taking these
bogus patches that contain variations on random

  #ifdef CONFIG_PREEMPT_RT

because they are clearly left-over turds from the RT tree that were
unbelievably ugly hacks, and should never have been merged upstream.

Why am I so upset?

WE ALREADY HAVE A DIFFERENT CONFIG VARIABLE EXPLICITLY FOR THIS!

In fact, you can *see* that config variable in the patch.

There's a very specific HAVE_SOFTIRQ_ON_OWN_STACK variable that has
the following help message (even if that help will never be shown
because it's not an actual question, it's a helper config variable
that gets selected):

   config HAVE_SOFTIRQ_ON_OWN_STACK
        bool
        help
          Architecture provides a function to run __do_softirq() on a
          separate stack.

and that config variable ALREADY PROTECTS the do_softirq_own_stack()
declaration in asm-generic. The very one you just added the
CONFIG_PREEMPT_RT thing around.

In other words, the RT patch is just mindless and ugly, and the right
thing to do would have been

 (a) make HAVE_SOFTIRQ_ON_OWN_STACK have a

        depends on !PREEMPT_RT

 (b) as PREEMPT_RT is enabled one architecture at a time, you can make
the architecture header files also use that HAVE_SOFTIRQ_ON_OWN_STACK
thing, which makes a whole lot more sense than sprinkling random
CONFIG_REEMPT_RT things around.

I have pulled this, but I'm really *really* fed up with these
PREEMPT_RT patches that add code that MAKES NO SENSE.

In just this merge window:

 We had it in the dentry tree.

 Then we had it in the printk tree to the point where I refused to even pull it.

 And now we have it in the asm-generic tree too.

The rule about RT patches has *always* been that we merge them as they
become clean enough to make sense. That rule seems to have entirely
flown out the window here, and suddenly it has become a sport to add
random senseless #ifdef CONFIG_PREEMPT_RT lines to code.

At least the dentry case had a nice big comment (which really was
required exactly because the code made no sense on its own). This
patch had nothing of the sort.

PREEMPT_RT is special enough that it really needs to spend a _lot_
more time making the code sensible, rather than add random hacks like
this. And when we have a config parameter that is *explicitly* about
this very issue, we should use that one, not some PREEMPT_RT hack.

And the RT tree has had literally decades where people tried very hard
to do exactly that - make proper abstractions, and make sure that
merging the RT patches made sense even outside the context of the RT
code.

Now suddenly all that "this code has to make sense" seems to be history.

And it really shouldn't be.

              Linus

Powered by blists - more mailing lists