Re: [RFC PATCH v2] Simply enable one to write code like:

Dmitry Torokhov
2024-10-01T15:21:15Z

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.

>
> 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 <andriy.shevchenko@intel.com>
> 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
> ---
> 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()?
> +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.

> @@ -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