Re: [RFC PATCH v2] Simply enable one to write code like:
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2024-10-01 15:21:15
Also in:
lkml
Hi Przemek, On Tue, Oct 01, 2024 at 04:57:18PM +0200, Przemek Kitszel wrote:
int foo(struct my_drv *adapter)
{
scoped_guard(spinlock, &adapter->some_spinlock)
return adapter->spinlock_protected_var;
}Could you change the subject to say something like: "Adjust cond_guard() implementation to avoid potential warnings" And then give detailed explanation in the body?
Current scoped_guard() implementation does not support that, due to compiler complaining: error: control reaches end of non-void function [-Werror=return-type] One could argue that for such use case it would be better to use guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say that coding with my proposed locking style is also very pleasant, as I'm doing so for a few weeks already.
I'd drop this paragraph from the patch description (and moved past "---" if you prefer to keep it for additional context.
quoted hunk ↗ jump to hunk
Technical stuff about the change: scoped_guard() macro uses common idiom of using "for" statement to declare a scoped variable. Unfortunately, current logic is too hard for compiler diagnostics to be sure that there is exactly one loop step; fix that. To make any loop so trivial that there is no above warning, it must not depend on any variable to tell if there are more steps. There is no obvious solution for that in C, but one could use the compound statement expression with "goto" jumping past the "loop", effectively leaving only the subscope part of the loop semantics. More impl details: one more level of macro indirection is now needed to avoid duplicating label names; I didn't spot any other place that is using if (0) past_the_loop:; else for (...; 1; ({goto past_the_loop})) idiom, so it's not packed for reuse what makes actual macros code cleaner. There was also a need to introduce const 0/1 variable per lock class, it is used to aid compiler diagnostics reasoning about "exactly 1 step" loops (note that converting that to function would undo the whole benefit). NAKed-by: Andy Shevchenko [off-list ref] Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> --- Andy believes that this change is completely wrong C, the reasons (that I disagree with of course, are in v1, below the commit message). v2: remove ", 1" condition, as scoped_guard() could be used also for conditional locks (try-lock, irq-lock, etc) - this was pointed out by Dmitry Torokhov and Dan Carpenter; reorder macros to have them defined prior to use - Markus Elfring. v1: https://lore.kernel.org/netdev/20240926134347.19371-1-przemyslaw.kitszel@intel.com (local) --- include/linux/cleanup.h | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h index a3d3e888cf1f..72dcfeb3ec13 100644 --- a/include/linux/cleanup.h +++ b/include/linux/cleanup.h@@ -151,12 +151,18 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ * */ + +#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond) \
This is not supposed to be used outside of cleanup.h so probably __DEFINE_CLASS_IS_CONDITIONAL()?
quoted hunk ↗ jump to hunk
+static __maybe_unused const bool class_##_name##_is_conditional = _is_cond + #define DEFINE_GUARD(_name, _type, _lock, _unlock) \ + DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \ DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \ static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \ { return *_T; } #define DEFINE_GUARD_COND(_name, _ext, _condlock) \ + DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1); \ EXTEND_CLASS(_name, _ext, \ ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \ class_##_name##_t _T) \@@ -167,10 +173,18 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ CLASS(_name, __UNIQUE_ID(guard)) #define __guard_ptr(_name) class_##_name##_lock_ptr +#define __is_cond_ptr(_name) class_##_name##_is_conditional + +#define scoped_guard(_name, args...) \ + __scoped_guard_labeled(__UNIQUE_ID(label), _name, args) -#define scoped_guard(_name, args...) \ - for (CLASS(_name, scope)(args), \ - *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1) +#define __scoped_guard_labeled(_label, _name, args...) \ + if (0) \ + _label: ; \ + else \ + for (CLASS(_name, scope)(args); \ + __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \ + ({goto _label;}))
The "jump back" throws me a little, do you think if can be rewritten as: if (true) for (...) else _label: /* dummy */ ;
#define scoped_cond_guard(_name, _fail, args...) \ for (CLASS(_name, scope)(args), \
With your __is_cond_ptr() can this be made to warn or error if scoped_cond_guard() is used with a non-conditional lock/class? As that would make no sense.
quoted hunk ↗ jump to hunk
@@ -233,14 +247,17 @@ static inline class_##_name##_t class_##_name##_constructor(void) \ } #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...) \ +DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \ __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__) \ __DEFINE_LOCK_GUARD_1(_name, _type, _lock) #define DEFINE_LOCK_GUARD_0(_name, _lock, _unlock, ...) \ +DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \ __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__) \ __DEFINE_LOCK_GUARD_0(_name, _lock) #define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock) \ + DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1); \ EXTEND_CLASS(_name, _ext, \ ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\ if (_T->lock && !(_condlock)) _T->lock = NULL; \base-commit: c824deb1a89755f70156b5cdaf569fca80698719 -- 2.39.3
Thanks. -- Dmitry