2022-03-21 22:03:10

by Ingo Molnar

[permalink] [raw]
Subject: [GIT PULL] locking changes for v5.18

Linus,

Please pull the latest locking/core git tree from:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2022-03-21

# HEAD: cd27ccfc727e99352321c0c75012ab9c5a90321e jump_label: Refactor #ifdef of struct static_key

Changes in this cycle were:

- bitops & cpumask:
- Always inline various generic helpers, to improve code generation,
but also for instrumentation, found by noinstr validation.
- Add a x86-specific cpumask_clear_cpu() helper to improve code generation

- atomics:
- Fix atomic64_{read_acquire,set_release} fallbacks

- lockdep:
- Fix /proc/lockdep output loop iteration for classes
- Fix /proc/lockdep potential access to invalid memory
- minor cleanups
- Add Mark Rutland as reviewer for atomic primitives

- jump labels:
- Clean up the code a bit

- misc:
- Add __sched annotations to percpu rwsem primitives
- Enable RT_MUTEXES on PREEMPT_RT by default
- Stray v8086_mode() inlining fix, result of noinstr objtool validation

Thanks,

Ingo

------------------>
Borislav Petkov (4):
asm-generic/bitops: Always inline all bit manipulation helpers
cpumask: Always inline helpers which use bit manipulation functions
cpumask: Add a x86-specific cpumask_clear_cpu() helper
x86/ptrace: Always inline v8086_mode() for instrumentation

Mark Rutland (2):
MAINTAINERS: add myself as reviewer for atomics
atomics: Fix atomic64_{read_acquire,set_release} fallbacks

Masahiro Yamada (2):
jump_label: Avoid unneeded casts in STATIC_KEY_INIT_{TRUE,FALSE}
jump_label: Refactor #ifdef of struct static_key

Minchan Kim (1):
locking: Add missing __sched attributes

Sebastian Andrzej Siewior (2):
locking/local_lock: Make the empty local_lock_*() function a macro.
locking: Enable RT_MUTEXES by default on PREEMPT_RT.

Waiman Long (2):
locking/lockdep: Avoid potential access of invalid memory in lock_class
locking/lockdep: Iterate lock_classes directly when reading lockdep files

Xiu Jianfeng (1):
lockdep: Use memset_startat() helper in reinit_class()


MAINTAINERS | 1 +
arch/x86/include/asm/cpumask.h | 10 +++++
arch/x86/include/asm/ptrace.h | 2 +-
include/asm-generic/bitops/instrumented-atomic.h | 12 ++---
.../asm-generic/bitops/instrumented-non-atomic.h | 16 +++----
include/linux/atomic/atomic-arch-fallback.h | 38 +++++++++++++---
include/linux/cpumask.h | 18 ++++----
include/linux/jump_label.h | 13 ++----
include/linux/local_lock_internal.h | 6 +--
init/Kconfig | 1 +
kernel/locking/lockdep.c | 43 ++++++++++--------
kernel/locking/lockdep_internals.h | 6 ++-
kernel/locking/lockdep_proc.c | 51 ++++++++++++++++++----
kernel/locking/percpu-rwsem.c | 5 ++-
kernel/locking/rwsem.c | 2 +-
scripts/atomic/fallbacks/read_acquire | 11 ++++-
scripts/atomic/fallbacks/set_release | 7 ++-
17 files changed, 168 insertions(+), 74 deletions(-)


2022-03-23 10:42:16

by pr-tracker-bot

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

The pull request you sent on Mon, 21 Mar 2022 12:11:44 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2022-03-21

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/ebd326ce724d5b2e5274724e6d6a46a046e28203

Thank you!

--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Subject: Re: [GIT PULL] locking changes for v5.18

On 2022-03-22 15:58:47 [-0700], Linus Torvalds wrote:
> On Tue, Mar 22, 2022 at 3:20 PM Borislav Petkov <[email protected]> wrote:
> >
> > Ah, you say build error because you have CONFIG_WERROR=y.
>
> EVERYBODY should have CONFIG_WERROR=y on at least x86-64 and other
> serious architectures, unless you have some completely random
> experimental (and broken) compiler.

I haven't seen it in my builds but it was reported earlier with a patch
https://lkml.kernel.org/r/[email protected]

which is sitting in Andrew's. I'm not completely satisfied with this
__maybe_unused, let me try to get clang and maybe I can figure something
else out.

> Linus

Sebastian

2022-03-23 15:06:28

by Borislav Petkov

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

+ Sebastian.

On Tue, Mar 22, 2022 at 03:05:39PM -0700, Linus Torvalds wrote:
> On Mon, Mar 21, 2022 at 4:11 AM Ingo Molnar <[email protected]> wrote:
> >
> > Sebastian Andrzej Siewior (2):
> > locking/local_lock: Make the empty local_lock_*() function a macro.
>
> Grr. I noticed this too late, but this one actually breaks the build with clang.
>
> Why?
>
> Because it's now a macro, it doesn't use the argument at all, and you get:
>
> mm/page_alloc.c:131:40: error: variable 'pagesets' is not needed
> and will not be emitted [-Werror,-Wunneeded-internal-declaration]
> static DEFINE_PER_CPU(struct pagesets, pagesets) = {
> ^
>
> and I'm not sure why this doesn't show up with gcc, but apparently gcc
> only warns about unused static functions, not unused static data.
>
> Or maybe gcc considers it used just because somebody did a typeof on it.
>
> I thought -tip had started checking with clang, but apparently not.

As a matter of fact, I do see this in my builds:

mm/page_alloc.c:131:40: warning: variable 'pagesets' is not needed and will not be emitted [-Wunneeded-internal-declaration]
static DEFINE_PER_CPU(struct pagesets, pagesets) = {
^
1 warning generated.

but I dismissed it as one of those not-in-tip-area warnings. Sorry about
that, I'll try to pay more attention in the future.

> I see that the -mm tree has a fix for this, but I'm rather unhappy
> that the -tip tree build checking has deteriorated so much, and clang
> builds will now have a pointless build error that will cause issues
> for bisect.

Ah, you say build error because you have CONFIG_WERROR=y.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Subject: [PATCH] locking/local_lock: Pretend to use the per-CPU variable if not needed.

In the !RT && !LOCKDEP case the per-CPU variables aren't used. The commit
mentioned below tried to avoid the usage of this_cpu_ptr() because it
generates code, clobbers registers which is not needed.
This change generated so little reference to the variable that llvm
assumed that it is not used and created a warning.

Revert local_lock_*() to its previous static inline implementation for
type checking.
Replace this_cpu_ptr() with __ll_cpu_ptr() which points to
this_cpu_ptr() when it is used.
In the !RT && !LOCKDEP case use per_cpu_ptr(, 0) which does not leave
any code behind and llvm does not complain either. It also ensures that
it is a per-CPU pointer. The assembly output in this case is unchanged.

Fixes: 9983a9d577db4 ("locking/local_lock: Make the empty local_lock_*() function a macro.")
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
include/linux/local_lock_internal.h | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index 6d635e8306d64..e671ead5fbad5 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -24,6 +24,8 @@ typedef struct {
}, \
.owner = NULL,

+#define __ll_cpu_ptr(__ll_cpuptr) (this_cpu_ptr(__ll_cpuptr))
+
static inline void local_lock_acquire(local_lock_t *l)
{
lock_map_acquire(&l->dep_map);
@@ -44,9 +46,10 @@ static inline void local_lock_debug_init(local_lock_t *l)
}
#else /* CONFIG_DEBUG_LOCK_ALLOC */
# define LOCAL_LOCK_DEBUG_INIT(lockname)
-# define local_lock_acquire(__ll) do { typecheck(local_lock_t *, __ll); } while (0)
-# define local_lock_release(__ll) do { typecheck(local_lock_t *, __ll); } while (0)
-# define local_lock_debug_init(__ll) do { typecheck(local_lock_t *, __ll); } while (0)
+# define __ll_cpu_ptr(__ll_cpuptr) per_cpu_ptr(__ll_cpuptr, 0)
+static inline void local_lock_acquire(local_lock_t *l) { }
+static inline void local_lock_release(local_lock_t *l) { }
+static inline void local_lock_debug_init(local_lock_t *l) { }
#endif /* !CONFIG_DEBUG_LOCK_ALLOC */

#define INIT_LOCAL_LOCK(lockname) { LOCAL_LOCK_DEBUG_INIT(lockname) }
@@ -65,36 +68,36 @@ do { \
#define __local_lock(lock) \
do { \
preempt_disable(); \
- local_lock_acquire(this_cpu_ptr(lock)); \
+ local_lock_acquire(__ll_cpu_ptr(lock)); \
} while (0)

#define __local_lock_irq(lock) \
do { \
local_irq_disable(); \
- local_lock_acquire(this_cpu_ptr(lock)); \
+ local_lock_acquire(__ll_cpu_ptr(lock)); \
} while (0)

#define __local_lock_irqsave(lock, flags) \
do { \
local_irq_save(flags); \
- local_lock_acquire(this_cpu_ptr(lock)); \
+ local_lock_acquire(__ll_cpu_ptr(lock)); \
} while (0)

#define __local_unlock(lock) \
do { \
- local_lock_release(this_cpu_ptr(lock)); \
+ local_lock_release(__ll_cpu_ptr(lock)); \
preempt_enable(); \
} while (0)

#define __local_unlock_irq(lock) \
do { \
- local_lock_release(this_cpu_ptr(lock)); \
+ local_lock_release(__ll_cpu_ptr(lock)); \
local_irq_enable(); \
} while (0)

#define __local_unlock_irqrestore(lock, flags) \
do { \
- local_lock_release(this_cpu_ptr(lock)); \
+ local_lock_release(__ll_cpu_ptr(lock)); \
local_irq_restore(flags); \
} while (0)

--
2.35.1

2022-03-23 23:36:15

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Mon, Mar 21, 2022 at 4:11 AM Ingo Molnar <[email protected]> wrote:
>
> Sebastian Andrzej Siewior (2):
> locking/local_lock: Make the empty local_lock_*() function a macro.

Grr. I noticed this too late, but this one actually breaks the build with clang.

Why?

Because it's now a macro, it doesn't use the argument at all, and you get:

mm/page_alloc.c:131:40: error: variable 'pagesets' is not needed
and will not be emitted [-Werror,-Wunneeded-internal-declaration]
static DEFINE_PER_CPU(struct pagesets, pagesets) = {
^

and I'm not sure why this doesn't show up with gcc, but apparently gcc
only warns about unused static functions, not unused static data.

Or maybe gcc considers it used just because somebody did a typeof on it.

I thought -tip had started checking with clang, but apparently not.

I see that the -mm tree has a fix for this, but I'm rather unhappy
that the -tip tree build checking has deteriorated so much, and clang
builds will now have a pointless build error that will cause issues
for bisect.

LInus

2022-03-23 23:38:19

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Tue, Mar 22, 2022 at 3:05 PM Linus Torvalds
<[email protected]> wrote:
>
> I see that the -mm tree has a fix for this,[..]

Ok, usually I strive to let the patch-bomb from Andrew sit in a branch
of its own for a while (in case somebody replies to one of Andrew's
emails I can then fix things up).

But I decided to just apply and merge the series immediately instead,
partly to just have this issue sorted out.

Let's hope there's nothing dodgy in there..

Linus

2022-03-24 00:25:49

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Tue, Mar 22, 2022 at 03:05:39PM -0700, Linus Torvalds wrote:

> I thought -tip had started checking with clang, but apparently not.

Yeah, sorry. I do indeed not regularly build with clang and somewhat
rely on the robots to yell at me.

Every time I try clang, it just takes so long.. build times are
significantly longer. On top of that our build system hasn't, until
recently [1], much liked how Debian packages clang.

[1] https://lkml.kernel.org/r/[email protected]

2022-03-24 01:09:50

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Tue, Mar 22, 2022 at 3:20 PM Borislav Petkov <[email protected]> wrote:
>
> Ah, you say build error because you have CONFIG_WERROR=y.

EVERYBODY should have CONFIG_WERROR=y on at least x86-64 and other
serious architectures, unless you have some completely random
experimental (and broken) compiler.

New compiler warnings are not acceptable.

Linus

2022-03-25 06:12:29

by Nick Desaulniers

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Thu, Mar 24, 2022 at 11:19:58AM +0100, Borislav Petkov wrote:
> On Thu, Mar 24, 2022 at 09:40:24AM +0100, Ingo Molnar wrote:
> > Only intermittently on my side - it only recently started working
> > reliably & it doubles the not inconsiderable test time :-/
>
> True dat - last time I measured, clang builds take roughly double the
> time gcc builds with the same config do.

See also:
https://lore.kernel.org/lkml/CAHk-=whs8QZf3YnifdLv57+FhBi5_WeNTG1B-suOES=RcUSmQg@mail.gmail.com/

We do have to pay a penalty in that if these binaries can be built as
shared objects, the MUST be. (Actually, IDK if Debian has the same
policy as Fedora).

If you see libLLVM-*.so (and libclang-cpp.so) in your list of
`ldd $(which clang)`, or see multiple symbols from the dynamic linker
after running:

$ perf record -e cycles:pp --call-graph lbr make LLVM=1
$ perf report --no-children --sort=dso,cycles

Then dynamic linking is the main source of performance difference
between GCC and Clang on those distributions. So we're kind of fighting
with one hand behind our backs.

For Linux distros that have server costs, my guess is these policies
allow them to minimize their costs of serving prebuilt binaries, since
dynamic linking does allow for smaller binaries.

For short lived processes like most of the toolchain (and parts that
generally don't overlap, such as the serialization between compilation
THEN linkages), dynamic linking provides a ridiculous startup cost. CoW
gets us most of the benefits of dynamic linking in terms of shared
memory between processes. (I don't think ASLR hurts CoW in that way?)

On the flip side, I have been banging pots and pans around internally
trying to raise awareness of the issue (I'm but a peasant though, Mr.
bottom rung of the ladder); we do have budget finally allocated to focus
on compiler performance. Getting the checks written involves more red
tape and more effort than I expected.

I might start ignoring compiler bugs for a while, go off into the
wilderness, and not come back until I have statically linked images of
clang available to host on kernel.org though. There's a lot we could be
doing to hot rod the compiler to optimize for developer's (and CI's)
time.

2022-03-25 14:06:00

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] locking/local_lock: Pretend to use the per-CPU variable if not needed.

On Wed, Mar 23, 2022 at 4:09 AM Sebastian Andrzej Siewior
<[email protected]> wrote:
>
> Replace this_cpu_ptr() with __ll_cpu_ptr() which points to
> this_cpu_ptr() when it is used.

Ok, so that's just really ugly.

Is there really no way to just fixthis_cpu_ptr() to not generate crap
code when the result isn't used?

I get the feeling that the real problem is that on x86, we have this:

#define arch_raw_cpu_ptr(ptr) \
({ \
unsigned long tcp_ptr__; \
asm volatile("add " __percpu_arg(1) ", %0" \
: "=r" (tcp_ptr__) \
: "m" (this_cpu_off), "0" (ptr)); \
(typeof(*(ptr)) __kernel __force *)tcp_ptr__; \
})

and that "volatile" is just *WRONG*.

That volatile is what literally tells the compiler "you can't remove
this if it isn't used".

But there's no point to that.

So how about we

(a) just revert commit 9983a9d577db4

(b) remove that bogus 'volatile'

Doesn't that fix the problem?

Linus

2022-03-25 18:06:40

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Tue, Mar 22, 2022 at 03:58:47PM -0700, Linus Torvalds wrote:
> On Tue, Mar 22, 2022 at 3:20 PM Borislav Petkov <[email protected]> wrote:
> >
> > Ah, you say build error because you have CONFIG_WERROR=y.
>
> EVERYBODY should have CONFIG_WERROR=y on at least x86-64 and other
> serious architectures, unless you have some completely random
> experimental (and broken) compiler.
>
> New compiler warnings are not acceptable.

What about old one? I have already complained in the early discussion that
`make W=1 ...` is broken by this change. Enabling it without fixing
_existing_ warnings on W=1 is not suitable for somebody. Now, I have to
modify my configs to disable WERROR because of inability to built at all.

(Yes, I understand that I may drop W=1, but that's not the point. since I
want to have clean builds of a new code on level 1 of warnings)

--
With Best Regards,
Andy Shevchenko


2022-03-25 19:06:37

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Fri, Mar 25, 2022 at 01:23:36PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 25, 2022 at 01:41:25PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 22, 2022 at 03:58:47PM -0700, Linus Torvalds wrote:
> > > On Tue, Mar 22, 2022 at 3:20 PM Borislav Petkov <[email protected]> wrote:
> > > >
> > > > Ah, you say build error because you have CONFIG_WERROR=y.
> > >
> > > EVERYBODY should have CONFIG_WERROR=y on at least x86-64 and other
> > > serious architectures, unless you have some completely random
> > > experimental (and broken) compiler.
> > >
> > > New compiler warnings are not acceptable.
> >
> > What about old one? I have already complained in the early discussion that
> > `make W=1 ...` is broken by this change. Enabling it without fixing
> > _existing_ warnings on W=1 is not suitable for somebody. Now, I have to
> > modify my configs to disable WERROR because of inability to built at all.
> >
> > (Yes, I understand that I may drop W=1, but that's not the point. since I
> > want to have clean builds of a new code on level 1 of warnings)
>
> It would be fairly easy to make scripts/Makefile.extrawarn strip out
> -Werror when W= is used.

Hmm... I can't achieve this, because it complains about recursive variable.
What helped me is to supply in such case -Wno-error which seems overrode
the previous setting.

I'll send a patch to discuss further if needed.

--
With Best Regards,
Andy Shevchenko


2022-03-25 19:34:06

by Ingo Molnar

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18


* Linus Torvalds <[email protected]> wrote:

> On Tue, Mar 22, 2022 at 3:05 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > I see that the -mm tree has a fix for this,[..]
>
> Ok, usually I strive to let the patch-bomb from Andrew sit in a branch
> of its own for a while (in case somebody replies to one of Andrew's
> emails I can then fix things up).
>
> But I decided to just apply and merge the series immediately instead,
> partly to just have this issue sorted out.
>
> Let's hope there's nothing dodgy in there..

Thanks!

> > I thought -tip had started checking with clang, but apparently not.

Only intermittently on my side - it only recently started working reliably
& it doubles the not inconsiderable test time :-/

> > I see that the -mm tree has a fix for this, but I'm rather unhappy that
> > the -tip tree build checking has deteriorated so much, and clang builds
> > will now have a pointless build error that will cause issues for
> > bisect.

Will try to add better clang testing, seems to have better warnings in a
couple of areas.

Thanks,

Ingo

Subject: [PATCH 0/3] Remove volatile from arch_raw_cpu_ptr() and revert the hacks.

On 2022-03-23 10:17:08 [-0700], Linus Torvalds wrote:
> I get the feeling that the real problem is that on x86, we have this:
>
> #define arch_raw_cpu_ptr(ptr) \
> ({ \
> unsigned long tcp_ptr__; \
> asm volatile("add " __percpu_arg(1) ", %0" \
> : "=r" (tcp_ptr__) \
> : "m" (this_cpu_off), "0" (ptr)); \
> (typeof(*(ptr)) __kernel __force *)tcp_ptr__; \
> })
>
> and that "volatile" is just *WRONG*.
>
> That volatile is what literally tells the compiler "you can't remove
> this if it isn't used".

It is indeed just x86. After double checking arm/mips removes that code
properly.

> But there's no point to that.
>
> So how about we
>
> (a) just revert commit 9983a9d577db4
>
> (b) remove that bogus 'volatile'
>
> Doesn't that fix the problem?

The following series does that. The assembly code looks okay. In a few
simple test cases the this_cpu_ptr() usage is always created and is not
moved passed preempt_enable() statement.
The resulting vmlinux shrunk a bit. The test config lost ~2KiB:
text data bss dec hex filename
22533901 10722831 13963496 47220228 2d08604 vmlinux.volatile
22531589 10722831 13971688 47226108 2d09cfc vmlinux.patched

after looking at it it was sometimes due avoiding this_cpu_ptr(),
sometimes it looked that the compiler made other decisions at the
earlier resulting to be more beneficial later on.

Sebastian


Subject: [PATCH 2/3] Revert "locking/local_lock: Make the empty local_lock_*() function a macro."

With volatile removed from arch_raw_cpu_ptr() the compiler no longer
creates the per-CPU reference. The usage of the macro can be reverted
now.

Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
include/linux/local_lock_internal.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index 6d635e8306d64..975e33b793a77 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -44,9 +44,9 @@ static inline void local_lock_debug_init(local_lock_t *l)
}
#else /* CONFIG_DEBUG_LOCK_ALLOC */
# define LOCAL_LOCK_DEBUG_INIT(lockname)
-# define local_lock_acquire(__ll) do { typecheck(local_lock_t *, __ll); } while (0)
-# define local_lock_release(__ll) do { typecheck(local_lock_t *, __ll); } while (0)
-# define local_lock_debug_init(__ll) do { typecheck(local_lock_t *, __ll); } while (0)
+static inline void local_lock_acquire(local_lock_t *l) { }
+static inline void local_lock_release(local_lock_t *l) { }
+static inline void local_lock_debug_init(local_lock_t *l) { }
#endif /* !CONFIG_DEBUG_LOCK_ALLOC */

#define INIT_LOCAL_LOCK(lockname) { LOCAL_LOCK_DEBUG_INIT(lockname) }
--
2.35.1

2022-03-25 19:51:53

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Fri, Mar 25, 2022 at 01:41:25PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 22, 2022 at 03:58:47PM -0700, Linus Torvalds wrote:
> > On Tue, Mar 22, 2022 at 3:20 PM Borislav Petkov <[email protected]> wrote:
> > >
> > > Ah, you say build error because you have CONFIG_WERROR=y.
> >
> > EVERYBODY should have CONFIG_WERROR=y on at least x86-64 and other
> > serious architectures, unless you have some completely random
> > experimental (and broken) compiler.
> >
> > New compiler warnings are not acceptable.
>
> What about old one? I have already complained in the early discussion that
> `make W=1 ...` is broken by this change. Enabling it without fixing
> _existing_ warnings on W=1 is not suitable for somebody. Now, I have to
> modify my configs to disable WERROR because of inability to built at all.
>
> (Yes, I understand that I may drop W=1, but that's not the point. since I
> want to have clean builds of a new code on level 1 of warnings)

It would be fairly easy to make scripts/Makefile.extrawarn strip out
-Werror when W= is used.

2022-03-25 19:58:43

by Borislav Petkov

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Thu, Mar 24, 2022 at 09:40:24AM +0100, Ingo Molnar wrote:
> Only intermittently on my side - it only recently started working
> reliably & it doubles the not inconsiderable test time :-/

True dat - last time I measured, clang builds take roughly double the
time gcc builds with the same config do.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2022-03-25 20:14:38

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Fri, Mar 25, 2022 at 4:42 AM Andy Shevchenko
<[email protected]> wrote:
>
> What about old one? I have already complained in the early discussion that
> `make W=1 ...` is broken by this change.

So that is REALLY D*MN EASY TO FIX.

If you use W=1, and don't want WERROR, then don't *do* that then.

End of story.

But that's on _you_. Not on the build system. If you use W=1 and
WERROR together, you get exactly what you asked for. It might even be
what you wanted, if you want to go through the warnings/errors as you
encounter them, instead of building everything.

And that's why I refuse to take the completely broken "strip out one
or the other automatically" change.

It's a perfectly valid combination to enable both.

But more importantly, -Werror is more important than W=1. So if
anything should be disabled, it's W=1.

Side note: that would be trivial to just have in the Kconfig files if
W=1 was just a config option.

Do something like

config EXTRA_ERRORS
int "Add extra compiler errors" if EXPERT
depends on !WERROR
range 0-2
default 0

but note again: WERROR should be the thing that controls this and
should be on by default, not the other way around.

If you want EXTRA_ERRORS, you should not only be CONFIG_EXPERT, you
should also have to manually disable WERROR that *normal* people
should have on by default.

Linus

2022-03-25 20:16:35

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [GIT PULL] locking changes for v5.18

On Fri, Mar 25, 2022 at 10:29:21AM -0700, Linus Torvalds wrote:
> On Fri, Mar 25, 2022 at 4:42 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > What about old one? I have already complained in the early discussion that
> > `make W=1 ...` is broken by this change.
>
> So that is REALLY D*MN EASY TO FIX.
>
> If you use W=1, and don't want WERROR, then don't *do* that then.
>
> End of story.
>
> But that's on _you_. Not on the build system. If you use W=1 and
> WERROR together, you get exactly what you asked for. It might even be
> what you wanted, if you want to go through the warnings/errors as you
> encounter them, instead of building everything.
>
> And that's why I refuse to take the completely broken "strip out one
> or the other automatically" change.
>
> It's a perfectly valid combination to enable both.
>
> But more importantly, -Werror is more important than W=1. So if
> anything should be disabled, it's W=1.
>
> Side note: that would be trivial to just have in the Kconfig files if
> W=1 was just a config option.
>
> Do something like
>
> config EXTRA_ERRORS
> int "Add extra compiler errors" if EXPERT
> depends on !WERROR
> range 0-2
> default 0
>
> but note again: WERROR should be the thing that controls this and
> should be on by default, not the other way around.
>
> If you want EXTRA_ERRORS, you should not only be CONFIG_EXPERT, you
> should also have to manually disable WERROR that *normal* people
> should have on by default.

I have got your point, thanks!

--
With Best Regards,
Andy Shevchenko