Re: [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards

["Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>]

On Tuesday, 30 January 2024 18:02:09 CET Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > Add cond_guard() to conditional guards.
> > 
> > cond_guard() is used for the _interruptible(), _killable(), and _try
> > versions of locks. It expects a block where the failure can be handled
> > (e.g., calling printk() and returning -EINTR in case of failure).
> > 
> > As the other guards, it avoids to open code the release of the lock
> > after a goto to an 'out' label.
> > 
> > This remains an RFC because Dan suggested a slightly different syntax:
> > 	if (cond_guard(...))
> > 	
> > 		return -EINTR;
> > 
> > But the scoped_cond_guard() macro omits the if statement:
> >     	scoped_cond_guard (...) {
> >     	}
> > 
> > Thus define cond_guard() similarly to scoped_cond_guard() but with a block
> > 
> > to handle the failure case:
> > 	cond_guard(...)
> > 	
> > 		return -EINTR;
> 
> That's too subtle for me, because of the mistakes that can be made with
> brackets how about a syntax like:
> 
>  	cond_guard(..., return -EINTR, ...)
> 
> ...to make it clear what happens if the lock acquisition fails without
> having to remember there is a hidden incomplete "if ()" statement in
> that macro? More below...

As you propose I can't see how to handle multi-line error path like in:

	cond_guard(...) {
		dev_dbg(...);
		return -EINTR;
	} 

I added a similar example in a comment in cleanup.h.

> 
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco
> > <fabio.maria.de.francesco@linux.intel.com> ---
> > 
> >  drivers/cxl/core/region.c | 17 +++++------------
> >  include/linux/cleanup.h   | 13 +++++++++++++
> >  2 files changed, 18 insertions(+), 12 deletions(-)
> > 
> > [...] 
> >
> > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > index c2d09bc4f976..fc850e61a47e 100644
> > --- a/include/linux/cleanup.h
> > +++ b/include/linux/cleanup.h
> > @@ -134,6 +134,15 @@ static inline class_##_name##_t
> > class_##_name##ext##_constructor(_init_args) \> 
> >   *	an anonymous instance of the (guard) class, not recommended for
> >   *	conditional locks.
> >   *
> > 
> > + * cond_guard(_name, args...):
> > + * 	for conditional locks like mutex_trylock() or
> > down_read_interruptible(). + * 	It expects a block for handling 
errors
> > like in the following example: + *
> > + * 	cond_guard(rwsem_read_intr, &my_sem) {
> > + * 		printk(KERN_NOTICE "Try failure in work0()\n");
> > + * 		return -EINTR;
> > + * 	}
> > + *
> > 
> >   * 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
> > 
> > @@ -165,6 +174,10 @@ static inline class_##_name##_t
> > class_##_name##ext##_constructor(_init_args) \> 
> >  #define __guard_ptr(_name) class_##_name##_lock_ptr
> > 
> > +#define cond_guard(_name, args...) \
> > +	CLASS(_name, scope)(args); \
> > +	if (!__guard_ptr(_name)(&scope))
> 
> This needs to protect against being used within another if () block.
> Imagine a case of:
> 
>     if (...) {
>     	cond_guard(...);
> 	    <statement>
>     } else if (...)
> 
> ...does that "else if" belong to the first "if ()" or the hidden one
> inside the macro?

Good question.

> You can steal the embedded "if ()" trick from scoped_cond_guard() and do
> something like (untested):
> 
> #define cond_guard(_name, _fail, args...) \
> 	CLASS(_name, scope)(args); \
> 	if (!__guard_ptr(_name)(&scope)) _fail; else /* pass */;

I think this may work, but...

Again, with this interface there is no means to handle multi-line error paths. 
I wanted an interface sufficiently flexible to handle logging + return -EINTR, 
and possibly more lines to undo something.

Can we have two macros, this for multi-line error paths, and one other, like 
you suggested, for an immediate 'return -EINTR'?

Thanks,

Fabio