[RFC,1/2] locking: Introduce __cleanup__ based guards

  Use __attribute__((__cleanup__(func))) to buid various guards:

 - ptr_guard()
 - void_guard() / void_scope()
 - lock_guard() / lock_scope()
 - double_lock_guard() / double_lock_scope()

Where the _guard thingies are variables with scope-based cleanup and
the _scope thingies are basically do-once for-loops with the same.

The CPP is rather impenetrable -- but I'll attempt to write proper
comments if/when people think this is worth pursuing.

Actual usage in the next patch

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/compiler_attributes.h |    2 
 include/linux/irqflags.h            |    7 ++
 include/linux/guards.h          |  118 ++++++++++++++++++++++++++++++++++++
 include/linux/mutex.h               |    5 +
 include/linux/preempt.h             |    4 +
 include/linux/rcupdate.h            |    3 
 include/linux/sched/task.h          |    2 
 include/linux/spinlock.h            |   23 +++++++
 8 files changed, 164 insertions(+)
  

On Fri, May 26, 2023 at 05:05:50PM +0200, Peter Zijlstra wrote:
> Use __attribute__((__cleanup__(func))) to buid various guards:
> 
>  - ptr_guard()
>  - void_guard() / void_scope()
>  - lock_guard() / lock_scope()
>  - double_lock_guard() / double_lock_scope()
> 
> Where the _guard thingies are variables with scope-based cleanup and
> the _scope thingies are basically do-once for-loops with the same.

This makes things much easier to deal with, rather than forcing loops
into separate functions, etc, and hoping to get the cleanup right.

> 
> The CPP is rather impenetrable -- but I'll attempt to write proper
> comments if/when people think this is worth pursuing.

Yes please. Comments would help a lot. I was scratching my head over _G
for a bit before I realized what was happening. :)

> 
> Actual usage in the next patch
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/compiler_attributes.h |    2 
>  include/linux/irqflags.h            |    7 ++
>  include/linux/guards.h          |  118 ++++++++++++++++++++++++++++++++++++
>  include/linux/mutex.h               |    5 +
>  include/linux/preempt.h             |    4 +
>  include/linux/rcupdate.h            |    3 
>  include/linux/sched/task.h          |    2 
>  include/linux/spinlock.h            |   23 +++++++
>  8 files changed, 164 insertions(+)
> 
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -366,4 +366,6 @@
>   */
>  #define __fix_address noinline __noclone
>  
> +#define __cleanup(func)			__attribute__((__cleanup__(func)))
> +
>  #endif /* __LINUX_COMPILER_ATTRIBUTES_H */

nitpick: sorting. This needs to be moved up alphabetically; the comment
at the start of the file says:

 ...
 * This file is meant to be sorted (by actual attribute name,
 * not by #define identifier). ...

> [...]
> +#define DEFINE_VOID_GUARD(_type, _Lock, _Unlock, ...)				\
> +typedef struct {								\
> +	__VA_ARGS__								\
> +} void_guard_##_type##_t;							\
> +										\
> [...]
> +DEFINE_VOID_GUARD(irq, local_irq_disable(), local_irq_enable())
> +DEFINE_VOID_GUARD(irqsave,
> +		  local_irq_save(_G->flags),
> +		  local_irq_restore(_G->flags),
> +		  unsigned long flags;)

Yeah, good trick for defining 0-or-more members to the guard struct. I
expect the common cases to be 0 or 1, so perhaps move the final ";" to
after __VA_ARGS__ to avoid needing it in the DEFINEs? (And even in this
initial patch, there's only 1 non-empty argument...)

> [...]
> --- /dev/null
> +++ b/include/linux/guards.h
> @@ -0,0 +1,118 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_GUARDS_H
> +#define __LINUX_GUARDS_H
> +
> +#include <linux/compiler_attributes.h>
> +
> +/* Pointer Guard */
> +
> +#define DEFINE_PTR_GUARD(_type, _Type, _Put)				\
> +typedef _Type *ptr_guard_##_type##_t;					\
> +static inline void ptr_guard_##_type##_cleanup(_Type **_ptr)		\
> +{									\
> +	_Type *_G = *_ptr;						\
> +	if (_G)								\
> +		_Put(_G);						\
> +}

*loud forehead-smacking noise* __cleanup with inlines! I love it!

> [...]
> +#define void_scope(_type)							\
> +	for (struct { void_guard_##_type##_t guard; bool done; } _scope		\
> +	     __cleanup(void_guard_##_type##_cleanup) =				\
> +	     { .guard = void_guard_##_type##_init() }; !_scope.done;		\
> +	     _scope.done = true)

Heh, yes, that'll work for a forced scope, and I bet compiler
optimizations will collapse a bunch of this into a very clean execution
path.

> [...]
> +DEFINE_VOID_GUARD(preempt, preempt_disable(), preempt_enable())
> +DEFINE_VOID_GUARD(migrate, migrate_disable(), migrate_enable())
> [...]
> +DEFINE_VOID_GUARD(rcu, rcu_read_lock(), rcu_read_unlock())
> [...]
> +DEFINE_PTR_GUARD(put_task, struct task_struct, put_task_struct)
> [...]

It seems like there are some _really_ common code patterns you're
targeting here, and I bet we could do some mechanical treewide changes
with Coccinelle to remove a ton of boilerplate code.

I like this API, and the CPP isn't very obfuscated at all, compared to
some stuff we've already got in the tree. :)

-Kees
  

On Fri, May 26, 2023 at 8:23 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> The CPP is rather impenetrable -- but I'll attempt to write proper
> comments if/when people think this is worth pursuing.

Ugh.

It's not only impenetrable, it seems _unnecessarily_ so.

Yes, yes, 'for()' loops can only declare one type, and if you want
multiple typed variables you declare a struct that contains all the
types.

But you don't actually *need* multiple types.

Yes, you think you do, because you want to use that 'bool done' to
make the for-loop only execute once. Nasty limitation of the for
syntax.

But you can actually do the 'bool done' using the exact same type you
have for the guard - just make it a pointer instead, and use NULL for
"not done" and non-NULL for "done". It ends up acting exactly like a
boolean.

But that extra structure is only a detail. The real ugliness comes
from using different scoping macros.

And I think you don't actually need to have those different forms of
"scoped()" macros for different cases. I think you can just use
variable macro arguments.

IOW, something like this:

  #define variable_scope(type, enter, exit) \
        for (type *_done = NULL, _scope __cleanup(exit) = enter;
!_done; _done = (void *)8)

  #define scoped(type, init...) \
        variable_scope(scope_##type##_t, scope_##type##_init(init),
scope_##type##_cleanup)

and then you can do

        scoped (rcu) {
                ...
        }

and it will call "scope_rcu_init()" on entry, and
"scope_rcu_exit(_scope)" on exit.

And just doing

        scoped (mutex, mymutex) { ... }

will call "scope_mytex_init(mymutex)" on entry, and
"scope_mytex_exit(_scope)" on exit.

And if you just make the scope_##type##_init() functions return the
right values, it all works very naturally.

I think you can also do things like

        scoped(irqsave) { ... }
        scoped(irqoff) { ... }
        scoped(preempt) { ... }

very naturally. No need for that odd "one scope for 'void', one scope
for 'lock'" nonsense.

I dunno. I didn't *test* the above. Maybe you already tried something
like the above, and there's a reason why it doesn't work.

             Linus
  

On Fri, May 26, 2023 at 7:05 PM Kees Cook <keescook@chromium.org> wrote:
>
> > --- a/include/linux/compiler_attributes.h
> > +++ b/include/linux/compiler_attributes.h
> > @@ -366,4 +366,6 @@
> >   */
> >  #define __fix_address noinline __noclone
> >
> > +#define __cleanup(func)                      __attribute__((__cleanup__(func)))
> > +
> >  #endif /* __LINUX_COMPILER_ATTRIBUTES_H */
>
> nitpick: sorting. This needs to be moved up alphabetically; the comment
> at the start of the file says:
>
>  ...
>  * This file is meant to be sorted (by actual attribute name,
>  * not by #define identifier). ...

+1, also please add:

    /*
     *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute
     * clang: https://clang.llvm.org/docs/AttributeReference.html#cleanup
     */

Cheers,
Miguel
  

On 5/26/23 11:05, Peter Zijlstra wrote:
> Use __attribute__((__cleanup__(func))) to buid various guards:
>
>   - ptr_guard()
>   - void_guard() / void_scope()
>   - lock_guard() / lock_scope()
>   - double_lock_guard() / double_lock_scope()
>
> Where the _guard thingies are variables with scope-based cleanup and
> the _scope thingies are basically do-once for-loops with the same.
>
> The CPP is rather impenetrable -- but I'll attempt to write proper
> comments if/when people think this is worth pursuing.
>
> Actual usage in the next patch
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>   include/linux/compiler_attributes.h |    2
>   include/linux/irqflags.h            |    7 ++
>   include/linux/guards.h          |  118 ++++++++++++++++++++++++++++++++++++
>   include/linux/mutex.h               |    5 +
>   include/linux/preempt.h             |    4 +
>   include/linux/rcupdate.h            |    3
>   include/linux/sched/task.h          |    2
>   include/linux/spinlock.h            |   23 +++++++
>   8 files changed, 164 insertions(+)

That is an interesting idea and may help to simplify some of the common 
code patterns that we have in the kernel. The macros are a bit hard to 
read and understand though I thought I got a rough idea of what they are 
trying to do.

BTW, do we have a use case for double_lock_guard/double_lock_scope? I 
can envision a nested lock_scope inside a lock_scope, but taking 2 auto 
locks of the same type at init time and then unlock them at exit just 
doesn't make sense to me.

Cheers,
Longman
  

On 5/26/23 14:49, Waiman Long wrote:
[...]
> 
> BTW, do we have a use case for double_lock_guard/double_lock_scope? I 
> can envision a nested lock_scope inside a lock_scope, but taking 2 auto 
> locks of the same type at init time and then unlock them at exit just 
> doesn't make sense to me.

AFAIU taking both runqueue locks for source and destination runqueues on 
migration is one use-case for double_lock_guard/scope.

Thanks,

Mathieu
  

On 5/26/23 14:58, Mathieu Desnoyers wrote:
> On 5/26/23 14:49, Waiman Long wrote:
> [...]
>>
>> BTW, do we have a use case for double_lock_guard/double_lock_scope? I 
>> can envision a nested lock_scope inside a lock_scope, but taking 2 
>> auto locks of the same type at init time and then unlock them at exit 
>> just doesn't make sense to me.
>
> AFAIU taking both runqueue locks for source and destination runqueues 
> on migration is one use-case for double_lock_guard/scope.
>
I see. Thanks for the clarification. I forgot about that special case.

Cheers,
Longman
  

On Fri, May 26, 2023 at 11:22:36AM -0700, Linus Torvalds wrote:

> But you can actually do the 'bool done' using the exact same type you
> have for the guard - just make it a pointer instead, and use NULL for
> "not done" and non-NULL for "done". It ends up acting exactly like a
> boolean.

Damn; I've actually seen that and should've thought of it.

> IOW, something like this:
> 
>   #define variable_scope(type, enter, exit) \
>         for (type *_done = NULL, _scope __cleanup(exit) = enter;
> !_done; _done = (void *)8)
> 
>   #define scoped(type, init...) \
>         variable_scope(scope_##type##_t, scope_##type##_init(init),
> scope_##type##_cleanup)
> 

> I dunno. I didn't *test* the above. Maybe you already tried something
> like the above, and there's a reason why it doesn't work.

I have not; let me go try that. That does look *much* nicer.