| Message ID | 20230526151946.960406324@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Lock and Pointer guards | expand |
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.
--- 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 */ --- a/include/linux/irqflags.h +++ b/include/linux/irqflags.h @@ -13,6 +13,7 @@ #define _LINUX_TRACE_IRQFLAGS_H #include <linux/typecheck.h> +#include <linux/guards.h> #include <asm/irqflags.h> #include <asm/percpu.h> @@ -267,4 +268,10 @@ extern void warn_bogus_irq_restore(void) #define irqs_disabled_flags(flags) raw_irqs_disabled_flags(flags) +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;) + #endif --- /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); \ +} + +#define ptr_guard(_type, _name) \ + ptr_guard_##_type##_t _name __cleanup(ptr_guard_##_type##_cleanup) + + +/* Void Guard */ + +#define DEFINE_VOID_GUARD(_type, _Lock, _Unlock, ...) \ +typedef struct { \ + __VA_ARGS__ \ +} void_guard_##_type##_t; \ + \ +static inline void void_guard_##_type##_cleanup(void *_g) \ +{ \ + void_guard_##_type##_t *_G __maybe_unused = _g; \ + _Unlock; \ +} \ + \ +static inline void_guard_##_type##_t void_guard_##_type##_init(void) \ +{ \ + void_guard_##_type##_t _g = { }, *_G __maybe_unused = &_g; \ + _Lock; \ + return _g; \ +} + +#define void_guard(_type, _name) \ + void_guard_##_type##_t _name __cleanup(void_guard_##_type##_cleanup) = \ + void_guard_##_type##_init() + +#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) + + +/* Lock Guard */ + +#define DEFINE_LOCK_GUARD(_type, _Type, _Lock, _Unlock, ...) \ +typedef struct { \ + _Type *lock; \ + __VA_ARGS__ \ +} lock_guard_##_type##_t; \ + \ +static inline void lock_guard_##_type##_cleanup(void *_g) \ +{ \ + lock_guard_##_type##_t *_G = _g; \ + _Unlock; \ +} \ + \ +static inline lock_guard_##_type##_t lock_guard_##_type##_init(_Type *lock) \ +{ \ + lock_guard_##_type##_t _g = { .lock = lock }, *_G = &_g; \ + _Lock; \ + return _g; \ +} + +#define lock_guard(_type, _name, _ptr) \ + lock_guard_##_type##_t _name __cleanup(lock_guard_##_type##_cleanup) = \ + lock_guard_##_type##_init(_ptr) + +#define lock_scope(_type, _ptr) \ + for (struct { lock_guard_##_type##_t guard; bool done; } _scope \ + __cleanup(lock_guard_##_type##_cleanup) = \ + { .guard = lock_guard_##_type##_init(_ptr) }; !_scope.done; \ + _scope.done = true) + + +/* Double Lock Guard */ + +#define DEFINE_DOUBLE_LOCK_GUARD(_type, _Type, _Lock, _Unlock, ...) \ +typedef struct { \ + _Type *lock; \ + _Type *lock2; \ + __VA_ARGS__ \ +} double_lock_guard_##_type##_t; \ + \ +static inline void double_lock_guard_##_type##_cleanup(void *_g) \ +{ \ + double_lock_guard_##_type##_t *_G = _g; \ + _Unlock; \ +} \ + \ +static inline double_lock_guard_##_type##_t double_lock_guard_##_type##_init(_Type *lock, _Type *lock2) \ +{ \ + double_lock_guard_##_type##_t _g = { .lock = lock, .lock2 = lock2 }, *_G = &_g;\ + _Lock; \ + return _g; \ +} + +#define double_lock_guard(_type, _name, _ptr, _ptr2) \ + double_lock_guard_##_type##_t _name __cleanup(double_lock_guard_##_type##_cleanup) = \ + double_lock_guard_##_type##_init(_ptr, _ptr2) + +#define double_lock_scope(_type, _ptr, _ptr2) \ + for (struct { double_lock_guard_##_type##_t guard; bool done; } _scope \ + __cleanup(double_lock_guard_##_type##_cleanup) = \ + { .guard = double_lock_guard_##_type##_init(_ptr, _ptr2) }; \ + !_scope.done; _scope.done = true) + + +#endif /* __LINUX_GUARDS_H */ --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -19,6 +19,7 @@ #include <asm/processor.h> #include <linux/osq_lock.h> #include <linux/debug_locks.h> +#include <linux/guards.h> #ifdef CONFIG_DEBUG_LOCK_ALLOC # define __DEP_MAP_MUTEX_INITIALIZER(lockname) \ @@ -219,4 +220,8 @@ extern void mutex_unlock(struct mutex *l extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock); +DEFINE_LOCK_GUARD(mutex, struct mutex, + mutex_lock(_G->lock), + mutex_unlock(_G->lock)) + #endif /* __LINUX_MUTEX_H */ --- a/include/linux/preempt.h +++ b/include/linux/preempt.h @@ -8,6 +8,7 @@ */ #include <linux/linkage.h> +#include <linux/guards.h> #include <linux/list.h> /* @@ -463,4 +464,7 @@ static __always_inline void preempt_enab preempt_enable(); } +DEFINE_VOID_GUARD(preempt, preempt_disable(), preempt_enable()) +DEFINE_VOID_GUARD(migrate, migrate_disable(), migrate_enable()) + #endif /* __LINUX_PREEMPT_H */ --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -27,6 +27,7 @@ #include <linux/preempt.h> #include <linux/bottom_half.h> #include <linux/lockdep.h> +#include <linux/guards.h> #include <asm/processor.h> #include <linux/cpumask.h> #include <linux/context_tracking_irq.h> @@ -1095,4 +1096,6 @@ rcu_head_after_call_rcu(struct rcu_head extern int rcu_expedited; extern int rcu_normal; +DEFINE_VOID_GUARD(rcu, rcu_read_lock(), rcu_read_unlock()) + #endif /* __LINUX_RCUPDATE_H */ --- a/include/linux/sched/task.h +++ b/include/linux/sched/task.h @@ -126,6 +126,8 @@ static inline void put_task_struct(struc __put_task_struct(t); } +DEFINE_PTR_GUARD(put_task, struct task_struct, put_task_struct) + static inline void put_task_struct_many(struct task_struct *t, int nr) { if (refcount_sub_and_test(nr, &t->usage)) --- a/include/linux/spinlock.h +++ b/include/linux/spinlock.h @@ -61,6 +61,7 @@ #include <linux/stringify.h> #include <linux/bottom_half.h> #include <linux/lockdep.h> +#include <linux/guards.h> #include <asm/barrier.h> #include <asm/mmiowb.h> @@ -502,5 +503,27 @@ int __alloc_bucket_spinlocks(spinlock_t void free_bucket_spinlocks(spinlock_t *locks); +DEFINE_LOCK_GUARD(raw, raw_spinlock_t, + raw_spin_lock(_G->lock), + raw_spin_unlock(_G->lock)) +DEFINE_LOCK_GUARD(raw_irq, raw_spinlock_t, + raw_spin_lock_irq(_G->lock), + raw_spin_unlock_irq(_G->lock)) +DEFINE_LOCK_GUARD(raw_irqsave, raw_spinlock_t, + raw_spin_lock_irqsave(_G->lock, _G->flags), + raw_spin_unlock_irqrestore(_G->lock, _G->flags), + unsigned long flags;) + +DEFINE_LOCK_GUARD(spin, spinlock_t, + spin_lock(_G->lock), + spin_unlock(_G->lock)) +DEFINE_LOCK_GUARD(spin_irq, spinlock_t, + spin_lock_irq(_G->lock), + spin_unlock_irq(_G->lock)) +DEFINE_LOCK_GUARD(spin_irqsave, spinlock_t, + spin_lock_irqsave(_G->lock, _G->flags), + spin_unlock_irqrestore(_G->lock, _G->flags), + unsigned long flags;) + #undef __LINUX_INSIDE_SPINLOCK_H #endif /* __LINUX_SPINLOCK_H */
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(+)