[GIT PULL] locking changes for v6.13

[GIT PULL] locking changes for v6.13

[Ingo Molnar]

Linus,

Please pull the latest locking/core Git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2024-11-18

   # HEAD: 3b49a347d751553b1d1be69c8619ae2e85fdc28d locking/Documentation: Fix grammar in percpu-rw-semaphore.rst

Locking changes for v6.13 are:

 - lockdep:
    - Enable PROVE_RAW_LOCK_NESTING with PROVE_LOCKING (Sebastian Andrzej Siewior)
    - Add lockdep_cleanup_dead_cpu() (David Woodhouse)

 - futexes:
    - Use atomic64_inc_return() in get_inode_sequence_number() (Uros Bizjak)
    - Use atomic64_try_cmpxchg_relaxed() in get_inode_sequence_number() (Uros Bizjak)

 - RT locking:
    - Add sparse annotation PREEMPT_RT's locking (Sebastian Andrzej Siewior)

 - spinlocks:
    - Use atomic_try_cmpxchg_release() in osq_unlock() (Uros Bizjak)

 - atomics:
    - x86: Use ALT_OUTPUT_SP() for __alternative_atomic64() (Uros Bizjak)
    - x86: Use ALT_OUTPUT_SP() for __arch_{,try_}cmpxchg64_emu() (Uros Bizjak)

 - KCSAN, seqlocks:
    - Support seqcount_latch_t (Marco Elver)

 - <linux/cleanup.h>:
    - Add if_not_cond_guard() conditional guard helper (David Lechner)
    - Adjust scoped_guard() macros to avoid potential warning (Przemek Kitszel)
    - Remove address space of returned pointer (Uros Bizjak)

 - WW mutexes:
    - locking/ww_mutex: Adjust to lockdep nest_lock requirements (Thomas Hellström)

 - Rust integration:
    - Fix raw_spin_lock initialization on PREEMPT_RT (Eder Zulian)

 - miscellaneous cleanups & fixes:
    - lockdep: Fix wait-type check related warnings (Ahmed Ehab)
    - lockdep: Use info level for initial info messages (Jiri Slaby)
    - spinlocks: Make __raw_* lock ops static (Geert Uytterhoeven)
    - pvqspinlock: Convert fields of 'enum vcpu_state' to uppercase (Qiuxu Zhuo)
    - iio: magnetometer: Fix if () scoped_guard() formatting (Stephen Rothwell)
    - rtmutex: Fix misleading comment (Peter Zijlstra)
    - percpu-rw-semaphores: Fix grammar in percpu-rw-semaphore.rst (Xiu Jianfeng)

 Thanks,

	Ingo

------------------>
Ahmed Ehab (2):
      locking/lockdep: Avoid creating new name string literals in lockdep_set_subclass()
      locking/lockdep: Add a test for lockdep_set_subclass()

David Lechner (1):
      cleanup: Add conditional guard helper

David Woodhouse (1):
      lockdep: Add lockdep_cleanup_dead_cpu()

Eder Zulian (1):
      rust: helpers: Avoid raw_spin_lock initialization for PREEMPT_RT

Geert Uytterhoeven (1):
      locking/spinlocks: Make __raw_* lock ops static

Jiri Slaby (SUSE) (1):
      lockdep: Use info level for lockdep initial info messages

Marco Elver (5):
      time/sched_clock: Swap update_clock_read_data() latch writes
      time/sched_clock: Broaden sched_clock()'s instrumentation coverage
      kcsan, seqlock: Support seqcount_latch_t
      seqlock, treewide: Switch to non-raw seqcount_latch interface
      kcsan, seqlock: Fix incorrect assumption in read_seqbegin()

Peter Zijlstra (1):
      locking/rtmutex: Fix misleading comment

Przemek Kitszel (1):
      cleanup: Adjust scoped_guard() macros to avoid potential warning

Qiuxu Zhuo (1):
      locking/pvqspinlock: Convert fields of 'enum vcpu_state' to uppercase

Sebastian Andrzej Siewior (5):
      lockdep: Enable PROVE_RAW_LOCK_NESTING with PROVE_LOCKING.
      locking/rt: Add sparse annotation PREEMPT_RT's sleeping locks.
      locking/rt: Remove one __cond_lock() in RT's spin_trylock_irqsave()
      locking/rt: Add sparse annotation for RCU.
      locking/rt: Annotate unlock followed by lock for sparse.

Stephen Rothwell (1):
      iio: magnetometer: fix if () scoped_guard() formatting

Thomas Hellström (1):
      locking/ww_mutex: Adjust to lockdep nest_lock requirements

Uros Bizjak (6):
      futex: Use atomic64_inc_return() in get_inode_sequence_number()
      futex: Use atomic64_try_cmpxchg_relaxed() in get_inode_sequence_number()
      cleanup: Remove address space of returned pointer
      locking/osq_lock: Use atomic_try_cmpxchg_release() in osq_unlock()
      locking/atomic/x86: Use ALT_OUTPUT_SP() for __alternative_atomic64()
      locking/atomic/x86: Use ALT_OUTPUT_SP() for __arch_{,try_}cmpxchg64_emu()

Xiu Jianfeng (1):
      locking/Documentation: Fix grammar in percpu-rw-semaphore.rst


 Documentation/locking/percpu-rw-semaphore.rst |  4 +-
 Documentation/locking/seqlock.rst             |  2 +-
 arch/x86/include/asm/atomic64_32.h            |  3 +-
 arch/x86/include/asm/cmpxchg_32.h             |  6 +-
 arch/x86/kernel/tsc.c                         |  5 +-
 drivers/iio/magnetometer/af8133j.c            |  3 +-
 include/linux/cleanup.h                       | 69 ++++++++++++++++---
 include/linux/irqflags.h                      |  6 ++
 include/linux/lockdep.h                       |  2 +-
 include/linux/rbtree_latch.h                  | 20 +++---
 include/linux/rwlock_rt.h                     | 10 +--
 include/linux/seqlock.h                       | 98 ++++++++++++++++++++-------
 include/linux/spinlock_rt.h                   | 28 ++++----
 include/linux/ww_mutex.h                      | 14 ++++
 kernel/cpu.c                                  |  1 +
 kernel/futex/core.c                           |  6 +-
 kernel/locking/lockdep.c                      | 46 ++++++++++---
 kernel/locking/osq_lock.c                     |  3 +-
 kernel/locking/qspinlock_paravirt.h           | 36 +++++-----
 kernel/locking/rtmutex.c                      |  2 +
 kernel/locking/rtmutex_api.c                  |  8 +--
 kernel/locking/spinlock.c                     |  8 +--
 kernel/locking/spinlock_rt.c                  | 14 ++--
 kernel/locking/test-ww_mutex.c                |  8 ++-
 kernel/printk/printk.c                        |  9 +--
 kernel/time/sched_clock.c                     | 34 +++++++---
 kernel/time/timekeeping.c                     | 12 ++--
 lib/Kconfig.debug                             | 12 +---
 lib/locking-selftest.c                        | 39 +++++++++++
 rust/helpers/spinlock.c                       |  8 ++-
 30 files changed, 355 insertions(+), 161 deletions(-)

Re: [GIT PULL] locking changes for v6.13

[Linus Torvalds]

On Mon, 18 Nov 2024 at 01:03, Ingo Molnar <mingo@kernel.org> wrote:
>
>  - <linux/cleanup.h>:
>     - Add if_not_cond_guard() conditional guard helper (David Lechner)

I've pulled this, but I'm unhappy.

This macro generates actively wrong code if it happens to be inside an
if-statement or a loop without a block.

IOW, code like this:

    for (iterate-over-something)
        if_not_guard(a)
            return -BUSY;

looks like will build fine, but will generate completely incorrect code.

Honestly, just switching the order of the BUILD_BUG_ON() and the
CLASS() declaration looks like it would have fixed this (because then
the '_id' won't be in scope of the subsequent if-statement any more),
but I'm unhappy with how apparently nobody even bothered to think
about such a fundamental issue with macros.

Macros that expand to statements absolutely *ALWAYS* need to deal with
"what if we're in a single-statement situation?"

              Linus

Re: [GIT PULL] locking changes for v6.13

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 18 Nov 2024 at 01:03, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >  - <linux/cleanup.h>:
> >     - Add if_not_cond_guard() conditional guard helper (David Lechner)
> 
> I've pulled this, but I'm unhappy.
> 
> This macro generates actively wrong code if it happens to be inside an
> if-statement or a loop without a block.
> 
> IOW, code like this:
> 
>     for (iterate-over-something)
>         if_not_guard(a)
>             return -BUSY;
> 
> looks like will build fine, but will generate completely incorrect code.
> 
> Honestly, just switching the order of the BUILD_BUG_ON() and the
> CLASS() declaration looks like it would have fixed this (because then
> the '_id' won't be in scope of the subsequent if-statement any more),
> but I'm unhappy with how apparently nobody even bothered to think
> about such a fundamental issue with macros.
> 
> Macros that expand to statements absolutely *ALWAYS* need to deal with
> "what if we're in a single-statement situation?"

Indeed - sorry about that, will sort this out tomorrow.

Thanks,

	Ingo

[PATCH] headers/cleanup.h: Fix if_not_guard() fragility

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 18 Nov 2024 at 01:03, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >  - <linux/cleanup.h>:
> >     - Add if_not_cond_guard() conditional guard helper (David Lechner)
> 
> I've pulled this, but I'm unhappy.
> 
> This macro generates actively wrong code if it happens to be inside an
> if-statement or a loop without a block.
> 
> IOW, code like this:
> 
>     for (iterate-over-something)
>         if_not_guard(a)
>             return -BUSY;
> 
> looks like will build fine, but will generate completely incorrect code.
> 
> Honestly, just switching the order of the BUILD_BUG_ON() and the
> CLASS() declaration looks like it would have fixed this (because then
> the '_id' won't be in scope of the subsequent if-statement any more),
> but I'm unhappy with how apparently nobody even bothered to think
> about such a fundamental issue with macros.
> 
> Macros that expand to statements absolutely *ALWAYS* need to deal with
> "what if we're in a single-statement situation?"

How about the fix below?

Thanks,

	Ingo

=======================>
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 20 Nov 2024 11:56:31 +0100
Subject: [PATCH] headers/cleanup.h: Fix if_not_guard() fragility

Linus noticed that the new if_not_guard() definition is fragile:

   "This macro generates actively wrong code if it happens to be inside an
    if-statement or a loop without a block.

    IOW, code like this:

      for (iterate-over-something)
          if_not_guard(a)
              return -BUSY;

    looks like will build fine, but will generate completely incorrect code."

The reason is that the __if_not_guard() macro is multi-statement, so 
while most kernel developers expect macros to be simple or at least 
compound statements - but for __if_not_guard() it is not so:

 #define __if_not_guard(_name, _id, args...)            \
        BUILD_BUG_ON(!__is_cond_ptr(_name));            \
        CLASS(_name, _id)(args);                        \
        if (!__guard_ptr(_name)(&_id))

To add insult to injury, the placement of the BUILD_BUG_ON() line makes 
the macro appear to compile fine, but it will generate incorrect code 
as Linus reported, for example if used within iteration or conditional 
statements that will use the first statement of a macro as a loop body 
or conditional statement body.

While it doesn't appear to be possible to turn this macro into a robust 
single or compound statement that could be used in single statements, 
due to the necessity to define an auto scope variable with an open 
scope and the necessity of it having to expand to a partial 'if' 
statement with no body - we can at least make sure the macro won't 
build if used in a single-statement construct: such as by making the 
CLASS() line the first statement in the macro, followed by the other 
statements, which would break the build, as the single statement would 
close the scope.

Do this.

To test this, I added an artificial if_not_guard() usecase within a 
single statement:

Before:

	$ make kernel/ptrace.o
	CC      kernel/ptrace.o
	$

After:

	CC      kernel/ptrace.o
	In file included from ./include/linux/irqflags.h:17,
		       from ./arch/x86/include/asm/special_insns.h:10,
		       from ./arch/x86/include/asm/processor.h:25,
		       from ./include/linux/sched.h:13,
		       from kernel/ptrace.c:13:
	kernel/ptrace.c: In function ‘ptrace_attach’:
	./include/linux/cleanup.h:258:9: error: expected expression before ‘class_mutex_intr_t’

I'd also like to note that the original submission by David Lechner did 
not contain the BUILD_BUG_ON() line, so it was safer than what we ended 
up committing. Mea culpa.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>
Fixes: 36c2cf88808d cleanup: Add conditional guard helper
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/cleanup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 966fcc5ff8ef..263f14085617 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -351,8 +351,8 @@ _label:									\
 	__scoped_cond_guard(_name, _fail, __UNIQUE_ID(label), args)
 
 #define __if_not_guard(_name, _id, args...)		\
-	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
 	CLASS(_name, _id)(args);			\
+	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
 	if (!__guard_ptr(_name)(&_id))
 
 #define if_not_guard(_name, args...) \

Re: [PATCH] headers/cleanup.h: Fix if_not_guard() fragility

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Mon, 18 Nov 2024 at 01:03, Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > >  - <linux/cleanup.h>:
> > >     - Add if_not_cond_guard() conditional guard helper (David Lechner)
> > 
> > I've pulled this, but I'm unhappy.
> > 
> > This macro generates actively wrong code if it happens to be inside an
> > if-statement or a loop without a block.
> > 
> > IOW, code like this:
> > 
> >     for (iterate-over-something)
> >         if_not_guard(a)
> >             return -BUSY;
> > 
> > looks like will build fine, but will generate completely incorrect code.
> > 
> > Honestly, just switching the order of the BUILD_BUG_ON() and the
> > CLASS() declaration looks like it would have fixed this (because then
> > the '_id' won't be in scope of the subsequent if-statement any more),
> > but I'm unhappy with how apparently nobody even bothered to think
> > about such a fundamental issue with macros.
> > 
> > Macros that expand to statements absolutely *ALWAYS* need to deal with
> > "what if we're in a single-statement situation?"
> 
> How about the fix below?

I also reviewed our other similar macros in <linux/cleanup.h>:

 - scoped_guard() appears to be single-statement safe: it uses a for() 
   statement with a partial body with an open 'else' branch, so if this 
   macro is used within single statements the entire block will be part 
   of the 'else' statement.

 - scoped_cond_guard(): similar construct to scoped_guard().

 - The other remaining multi-statement macros are variable definition 
   macros (DEFINE_CLASS(), et al), which are typically used in file 
   scope or in header scope, and are not expected to be used in single 
   statements.

So it appears to me we should be OK wrt. this class of bugs?

Thanks,

	Ingo

Re: [PATCH] headers/cleanup.h: Fix if_not_guard() fragility

[David Lechner]

On 11/20/24 5:36 AM, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Mon, 18 Nov 2024 at 01:03, Ingo Molnar <mingo@kernel.org> wrote:
>>>
>>>  - <linux/cleanup.h>:
>>>     - Add if_not_cond_guard() conditional guard helper (David Lechner)
>>
>> I've pulled this, but I'm unhappy.
>>
>> This macro generates actively wrong code if it happens to be inside an
>> if-statement or a loop without a block.
>>
>> IOW, code like this:
>>
>>     for (iterate-over-something)
>>         if_not_guard(a)
>>             return -BUSY;
>>
>> looks like will build fine, but will generate completely incorrect code.
>>
>> Honestly, just switching the order of the BUILD_BUG_ON() and the
>> CLASS() declaration looks like it would have fixed this (because then
>> the '_id' won't be in scope of the subsequent if-statement any more),
>> but I'm unhappy with how apparently nobody even bothered to think
>> about such a fundamental issue with macros.
>>
>> Macros that expand to statements absolutely *ALWAYS* need to deal with
>> "what if we're in a single-statement situation?"
> 
> How about the fix below?
> 
> Thanks,
> 
> 	Ingo
> 
> =======================>
> From: Ingo Molnar <mingo@kernel.org>
> Date: Wed, 20 Nov 2024 11:56:31 +0100
> Subject: [PATCH] headers/cleanup.h: Fix if_not_guard() fragility
> 
> Linus noticed that the new if_not_guard() definition is fragile:
> 
>    "This macro generates actively wrong code if it happens to be inside an
>     if-statement or a loop without a block.
> 
>     IOW, code like this:
> 
>       for (iterate-over-something)
>           if_not_guard(a)
>               return -BUSY;
> 
>     looks like will build fine, but will generate completely incorrect code."
> 
> The reason is that the __if_not_guard() macro is multi-statement, so 
> while most kernel developers expect macros to be simple or at least 
> compound statements - but for __if_not_guard() it is not so:
> 
>  #define __if_not_guard(_name, _id, args...)            \
>         BUILD_BUG_ON(!__is_cond_ptr(_name));            \
>         CLASS(_name, _id)(args);                        \
>         if (!__guard_ptr(_name)(&_id))
> 
> To add insult to injury, the placement of the BUILD_BUG_ON() line makes 
> the macro appear to compile fine, but it will generate incorrect code 
> as Linus reported, for example if used within iteration or conditional 
> statements that will use the first statement of a macro as a loop body 
> or conditional statement body.
> 
> While it doesn't appear to be possible to turn this macro into a robust 
> single or compound statement that could be used in single statements, 
> due to the necessity to define an auto scope variable with an open 
> scope and the necessity of it having to expand to a partial 'if' 
> statement with no body - we can at least make sure the macro won't 
> build if used in a single-statement construct: such as by making the 
> CLASS() line the first statement in the macro, followed by the other 
> statements, which would break the build, as the single statement would 
> close the scope.

Here is another option:

We could scrap this macro and try a different approach completely.
Instead we could create something that works a bit different but is
actually a single C statement.

Instead of this code...

	if_not_guard(mutex_intr, &st->lock)
		return -EINTR;

We could write this...

	int ret;
	
	cond_guard(mutex_intr, &st->lock, &ret);
	if (ret)
		return ret;

In this case, the cond_guard() macro would expand to a single statement,
namely a variable declaration statement.

This would also fix another thing that bugged me about the existing
scoped_cond_guard() that this is aiming to replace. scoped_cond_guard()
swallows the return value of the acquire function and just returns a
handle or NULL, possibly losing information.

In fact, mutex_lock_interruptible() that I used in this example can
return -EINTR, -EALREADY, or -EDEADLK. This means that patches like
[1] are actually unintentionally changing behavior because instead of
passing on the return value, they assume that only -EINTR could be
returned and hard-code that. This will cause bugs if anyone higher up
the call stack that is checking for a specific error code. If we want
to fix if_cond_guard() we should make it robust against this mistake
as well. But at this point, I think reverting my patch and going
back to the drawing board is the best option.

[1]: https://lore.kernel.org/all/20240904043104.1030257-2-dmitry.torokhov@gmail.com/


Note: We can't make the most obvious macro that works like this...

	ret = cond_guard(mutex_intr, &st->lock);

for the same reason that if_not_guard() is destined to be buggy - there
just isn't a way to do it in a single statement/expression while keeping
the cleanup variable declaration in the current scope. For this reason
I am proposing the next best thing where ret is an output parameter.

> 
> Do this.
> 
> To test this, I added an artificial if_not_guard() usecase within a 
> single statement:
> 
> Before:
> 
> 	$ make kernel/ptrace.o
> 	CC      kernel/ptrace.o
> 	$
> 
> After:
> 
> 	CC      kernel/ptrace.o
> 	In file included from ./include/linux/irqflags.h:17,
> 		       from ./arch/x86/include/asm/special_insns.h:10,
> 		       from ./arch/x86/include/asm/processor.h:25,
> 		       from ./include/linux/sched.h:13,
> 		       from kernel/ptrace.c:13:
> 	kernel/ptrace.c: In function ‘ptrace_attach’:
> 	./include/linux/cleanup.h:258:9: error: expected expression before ‘class_mutex_intr_t’
> 
> I'd also like to note that the original submission by David Lechner did 
> not contain the BUILD_BUG_ON() line, so it was safer than what we ended 
> up committing. Mea culpa.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: David Lechner <dlechner@baylibre.com>
> Fixes: 36c2cf88808d cleanup: Add conditional guard helper
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/cleanup.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 966fcc5ff8ef..263f14085617 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -351,8 +351,8 @@ _label:									\
>  	__scoped_cond_guard(_name, _fail, __UNIQUE_ID(label), args)
>  
>  #define __if_not_guard(_name, _id, args...)		\
> -	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
>  	CLASS(_name, _id)(args);			\
> +	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
>  	if (!__guard_ptr(_name)(&_id))
>  
>  #define if_not_guard(_name, args...) \

Re: [PATCH] headers/cleanup.h: Fix if_not_guard() fragility

[Linus Torvalds]

On Wed, 20 Nov 2024 at 09:57, David Lechner <dlechner@baylibre.com> wrote:
>
>         cond_guard(mutex_intr, &st->lock, &ret);
>         if (ret)
>                 return ret;

I'm not convinced that improves on anything.

You just replace one disgusting syntax with another, and force people
to have a variable that they may not want to have (even if they have
an error return variable, it might commonly be an error pointer, for
example)

I really think the basic issue is that "cond_guard" itself is a pretty
broken concept. It simply doesn't work very well in the C syntax.

I wish people just gave up on it entirely rather than try to work
around that fundamental fact.

Not that long ago, Mathieu wanted to introduce "inactive guards" for
some similar reasons - kind of "conditional guards, except the
conditional is outside the guard". And I pointed out that the fix was
to rewrite the disgusting code so that THEY WEREN'T NEEDED in the
place he wanted to use them. Rewriting things to "Just Don't Do That,
Then" actually just improved code entirely:

   https://lore.kernel.org/all/CAHk-=wgRefOSUy88-rcackyb4Ss3yYjuqS_TJRJwY_p7E3r0SA@mail.gmail.com/

and honestly, I suspect the same is often true of this whole
"if_not_guard()" thing. It's not *hugely* often needed, and I strongly
suspect that the explicitly scoped version would be a *lot* safer.

The "if_not_guard()" model may be great for mindless conversions of
existing code. But I'm not convinced it's a great interface in itself,
or that "mindless conversions" of conditional locking is actually a
good thing.

          Linus

[PATCH] headers/cleanup.h: Remove the if_not_guard() facility

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 20 Nov 2024 at 09:57, David Lechner <dlechner@baylibre.com> wrote:
> >
> >         cond_guard(mutex_intr, &st->lock, &ret);
> >         if (ret)
> >                 return ret;
> 
> I'm not convinced that improves on anything.
> 
> You just replace one disgusting syntax with another, and force people
> to have a variable that they may not want to have (even if they have
> an error return variable, it might commonly be an error pointer, for
> example)
> 
> I really think the basic issue is that "cond_guard" itself is a pretty
> broken concept. It simply doesn't work very well in the C syntax.
> 
> I wish people just gave up on it entirely rather than try to work
> around that fundamental fact.
> 
> Not that long ago, Mathieu wanted to introduce "inactive guards" for
> some similar reasons - kind of "conditional guards, except the
> conditional is outside the guard". And I pointed out that the fix was
> to rewrite the disgusting code so that THEY WEREN'T NEEDED in the
> place he wanted to use them. Rewriting things to "Just Don't Do That,
> Then" actually just improved code entirely:
> 
>    https://lore.kernel.org/all/CAHk-=wgRefOSUy88-rcackyb4Ss3yYjuqS_TJRJwY_p7E3r0SA@mail.gmail.com/
> 
> and honestly, I suspect the same is often true of this whole
> "if_not_guard()" thing. It's not *hugely* often needed, and I strongly
> suspect that the explicitly scoped version would be a *lot* safer.
> 
> The "if_not_guard()" model may be great for mindless conversions of
> existing code. But I'm not convinced it's a great interface in itself,
> or that "mindless conversions" of conditional locking is actually a
> good thing.

Ok, agreed - and to progress with fixing the bug & the fragility you 
noticed, let's remove if_cond_guard() as a first step via the patch 
below.

Thanks,

	Ingo

=================================>
From: Ingo Molnar <mingo@kernel.org>
Date: Fri, 6 Dec 2024 10:13:32 +0100
Subject: [PATCH] headers/cleanup.h: Remove the if_not_guard() facility

Linus noticed that the new if_not_guard() definition is fragile:

   "This macro generates actively wrong code if it happens to be inside an
    if-statement or a loop without a block.

    IOW, code like this:

      for (iterate-over-something)
          if_not_guard(a)
              return -BUSY;

    looks like will build fine, but will generate completely incorrect code."

The reason is that the __if_not_guard() macro is multi-statement, so
while most kernel developers expect macros to be simple or at least
compound statements - but for __if_not_guard() it is not so:

 #define __if_not_guard(_name, _id, args...)            \
        BUILD_BUG_ON(!__is_cond_ptr(_name));            \
        CLASS(_name, _id)(args);                        \
        if (!__guard_ptr(_name)(&_id))

To add insult to injury, the placement of the BUILD_BUG_ON() line makes
the macro appear to compile fine, but it will generate incorrect code
as Linus reported, for example if used within iteration or conditional
statements that will use the first statement of a macro as a loop body
or conditional statement body.

It doesn't appear to be possible to turn this macro into a robust
single or compound statement that could be used in single statements,
due to the necessity to define an auto scope variable with an open
scope and the necessity of it having to expand to a partial 'if'
statement with no body.

Instead of trying to work around this fragility, just remove the
construct before it gets used by code.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>
---
 include/linux/cleanup.h | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 966fcc5ff8ef..ec00e3f7af2b 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -273,12 +273,6 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *	an anonymous instance of the (guard) class, not recommended for
  *	conditional locks.
  *
- * if_not_guard(name, args...) { <error handling> }:
- *	convenience macro for conditional guards that calls the statement that
- *	follows only if the lock was not acquired (typically an error return).
- *
- *	Only for conditional locks.
- *
  * scoped_guard (name, args...) { }:
  *	similar to CLASS(name, scope)(args), except the variable (with the
  *	explicit name 'scope') is declard in a for-loop such that its scope is
@@ -350,14 +344,6 @@ _label:									\
 #define scoped_cond_guard(_name, _fail, args...)	\
 	__scoped_cond_guard(_name, _fail, __UNIQUE_ID(label), args)
 
-#define __if_not_guard(_name, _id, args...)		\
-	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
-	CLASS(_name, _id)(args);			\
-	if (!__guard_ptr(_name)(&_id))
-
-#define if_not_guard(_name, args...) \
-	__if_not_guard(_name, __UNIQUE_ID(guard), args)
-
 /*
  * Additional helper macros for generating lock guards with types, either for
  * locks that don't have a native type (eg. RCU, preempt) or those that need a

Re: [PATCH] headers/cleanup.h: Remove the if_not_guard() facility

[David Lechner]

On 12/6/24 3:19 AM, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Wed, 20 Nov 2024 at 09:57, David Lechner <dlechner@baylibre.com> wrote:
>>>
>>>         cond_guard(mutex_intr, &st->lock, &ret);
>>>         if (ret)
>>>                 return ret;
>>
>> I'm not convinced that improves on anything.
>>
>> You just replace one disgusting syntax with another, and force people
>> to have a variable that they may not want to have (even if they have
>> an error return variable, it might commonly be an error pointer, for
>> example)
>>
>> I really think the basic issue is that "cond_guard" itself is a pretty
>> broken concept. It simply doesn't work very well in the C syntax.
>>
>> I wish people just gave up on it entirely rather than try to work
>> around that fundamental fact.
>>
>> Not that long ago, Mathieu wanted to introduce "inactive guards" for
>> some similar reasons - kind of "conditional guards, except the
>> conditional is outside the guard". And I pointed out that the fix was
>> to rewrite the disgusting code so that THEY WEREN'T NEEDED in the
>> place he wanted to use them. Rewriting things to "Just Don't Do That,
>> Then" actually just improved code entirely:
>>
>>    https://lore.kernel.org/all/CAHk-=wgRefOSUy88-rcackyb4Ss3yYjuqS_TJRJwY_p7E3r0SA@mail.gmail.com/
>>
>> and honestly, I suspect the same is often true of this whole
>> "if_not_guard()" thing. It's not *hugely* often needed, and I strongly
>> suspect that the explicitly scoped version would be a *lot* safer.
>>
>> The "if_not_guard()" model may be great for mindless conversions of
>> existing code. But I'm not convinced it's a great interface in itself,
>> or that "mindless conversions" of conditional locking is actually a
>> good thing.
> 
> Ok, agreed - and to progress with fixing the bug & the fragility you 
> noticed, let's remove if_cond_guard() as a first step via the patch 
> below.
> 
> Thanks,
> 
> 	Ingo
> 
> =================================>
> From: Ingo Molnar <mingo@kernel.org>
> Date: Fri, 6 Dec 2024 10:13:32 +0100
> Subject: [PATCH] headers/cleanup.h: Remove the if_not_guard() facility
> 
> Linus noticed that the new if_not_guard() definition is fragile:
> 
>    "This macro generates actively wrong code if it happens to be inside an
>     if-statement or a loop without a block.
> 
>     IOW, code like this:
> 
>       for (iterate-over-something)
>           if_not_guard(a)
>               return -BUSY;
> 
>     looks like will build fine, but will generate completely incorrect code."
> 
> The reason is that the __if_not_guard() macro is multi-statement, so
> while most kernel developers expect macros to be simple or at least
> compound statements - but for __if_not_guard() it is not so:
> 
>  #define __if_not_guard(_name, _id, args...)            \
>         BUILD_BUG_ON(!__is_cond_ptr(_name));            \
>         CLASS(_name, _id)(args);                        \
>         if (!__guard_ptr(_name)(&_id))
> 
> To add insult to injury, the placement of the BUILD_BUG_ON() line makes
> the macro appear to compile fine, but it will generate incorrect code
> as Linus reported, for example if used within iteration or conditional
> statements that will use the first statement of a macro as a loop body
> or conditional statement body.
> 
> It doesn't appear to be possible to turn this macro into a robust
> single or compound statement that could be used in single statements,
> due to the necessity to define an auto scope variable with an open
> scope and the necessity of it having to expand to a partial 'if'
> statement with no body.
> 
> Instead of trying to work around this fragility, just remove the
> construct before it gets used by code.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: David Lechner <dlechner@baylibre.com>
> ---
I agree this is the right thing to do. Thanks for writing up the patch.

Acked-by: David Lechner <dlechner@baylibre.com>

[tip: locking/urgent] headers/cleanup.h: Remove the if_not_guard() facility

[tip-bot2 for Ingo Molnar]

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     b4d83c8323b0c4a899a996fed919cfe10720d289
Gitweb:        https://git.kernel.org/tip/b4d83c8323b0c4a899a996fed919cfe10720d289
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Fri, 06 Dec 2024 10:19:31 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sat, 07 Dec 2024 11:15:14 +01:00

headers/cleanup.h: Remove the if_not_guard() facility

Linus noticed that the new if_not_guard() definition is fragile:

   "This macro generates actively wrong code if it happens to be inside an
    if-statement or a loop without a block.

    IOW, code like this:

      for (iterate-over-something)
          if_not_guard(a)
              return -BUSY;

    looks like will build fine, but will generate completely incorrect code."

The reason is that the __if_not_guard() macro is multi-statement, so
while most kernel developers expect macros to be simple or at least
compound statements - but for __if_not_guard() it is not so:

 #define __if_not_guard(_name, _id, args...)            \
        BUILD_BUG_ON(!__is_cond_ptr(_name));            \
        CLASS(_name, _id)(args);                        \
        if (!__guard_ptr(_name)(&_id))

To add insult to injury, the placement of the BUILD_BUG_ON() line makes
the macro appear to compile fine, but it will generate incorrect code
as Linus reported, for example if used within iteration or conditional
statements that will use the first statement of a macro as a loop body
or conditional statement body.

[ I'd also like to note that the original submission by David Lechner did
  not contain the BUILD_BUG_ON() line, so it was safer than what we ended
  up committing. Mea culpa. ]

It doesn't appear to be possible to turn this macro into a robust
single or compound statement that could be used in single statements,
due to the necessity to define an auto scope variable with an open
scope and the necessity of it having to expand to a partial 'if'
statement with no body.

Instead of trying to work around this fragility, just remove the
construct before it gets used.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/Z1LBnX9TpZLR5Dkf@gmail.com
---
 include/linux/cleanup.h | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 966fcc5..ec00e3f 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -273,12 +273,6 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *	an anonymous instance of the (guard) class, not recommended for
  *	conditional locks.
  *
- * if_not_guard(name, args...) { <error handling> }:
- *	convenience macro for conditional guards that calls the statement that
- *	follows only if the lock was not acquired (typically an error return).
- *
- *	Only for conditional locks.
- *
  * scoped_guard (name, args...) { }:
  *	similar to CLASS(name, scope)(args), except the variable (with the
  *	explicit name 'scope') is declard in a for-loop such that its scope is
@@ -350,14 +344,6 @@ _label:									\
 #define scoped_cond_guard(_name, _fail, args...)	\
 	__scoped_cond_guard(_name, _fail, __UNIQUE_ID(label), args)
 
-#define __if_not_guard(_name, _id, args...)		\
-	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
-	CLASS(_name, _id)(args);			\
-	if (!__guard_ptr(_name)(&_id))
-
-#define if_not_guard(_name, args...) \
-	__if_not_guard(_name, __UNIQUE_ID(guard), args)
-
 /*
  * Additional helper macros for generating lock guards with types, either for
  * locks that don't have a native type (eg. RCU, preempt) or those that need a

Re: [GIT PULL] locking changes for v6.13

[pr-tracker-bot]

The pull request you sent on Mon, 18 Nov 2024 10:03:40 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2024-11-18

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

Thank you!

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