Re: [PATCH 1/2 v4] cleanup: Add cond_guard() to conditional guards

[Fabio M. De Francesco]

On Thursday, 15 February 2024 11:26:42 CET Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 18:04:52 +0000
> 
> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> > On Tue, 13 Feb 2024 17:51:26 +0100
> > 
> > "Fabio M. De Francesco" <fabio.maria.de.francesco@xxxxxxxxxxxxxxx> wrote:
> > > On Thursday, 8 February 2024 14:04:23 CET Fabio M. De Francesco wrote:
> > > > Add cond_guard() macro to conditional guards.
> > > > 
> > > > cond_guard() is a guard to be used with the conditional variants of
> > > > locks,
> > > > like down_read_trylock() or mutex_lock_interruptible().
> > > > 
> > > > It takes a statement (or statement-expression) that is passed as its
> > > > second argument. That statement (or statement-expression) is executed
> > > > if
> > > > waiting for a lock is interrupted or if a _trylock() fails in case of
> > > > contention.
> > > > 
> > > > Usage example:
> > > > 	cond_guard(mutex_intr, return -EINTR, &mutex);
> > > > 
> > > > Consistent with other usage of _guard(), locks are unlocked at the
> > > > exit of
> > > > the scope where cond_guard() is called.
> > > 
> > > [snip]
> > > 
> > > > +#define cond_guard(_name, _fail, args...) \
> > > > +	CLASS(_name, scope)(args); \
> > > > +	if (!__guard_ptr(_name)(&scope)) _fail; \
> > > > +	else { }
> > > > +
> > > 
> > > I have converted and tested several functions in drivers/cxl and found
> > > that
> > > there are cases where this macro needs to be called twice in the same
> > > scope.
> > > 
> > > The current implementation fails to compile because any subsequent call
> > > to
> > > cond_guard() redefines "scope".
> > > 
> > > I have a solution for this, which is to instantiate a differently named
> > > variable each time cond_guard() is used:
> > > 
> > > #define __UNIQUE_LINE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
> > > __LINE__) #define cond_guard(_name, _fail, args...) \
> > > 
> > >         CLASS(_name, __UNIQUE_LINE_ID(scope))(args); \
> > >         if (!__guard_ptr(_name)(&__UNIQUE_LINE_ID(scope))) _fail; \
> > >         else { }
> > > 
> > > But, before sending v5, I think it's best to wait for comments from
> > > those with more experience than me.
> > 
> > Ah. So you can't use __UNIQUE_ID as guard does because we need it to be
> > stable across the two uses.  What you have looks fine to me.
> > We might end up with someone putting multiple calls in a macro but in my
> > view anyone doing that level of complexity in a macro is shooting
> > themselves in the foot.
> 
> Thought more on this whilst cycling home.  Can you use another level
> of macros in combination with __UNIQUE_ID that guard uses?
> My skills with macros are very limited so I'm sure I got something wrong,
> but along the lines of.
> 
> #define __cond_class(__unique, _name, _fail, args...) \
>    CLASS(_name, __unique)(args); \
>          if (!__guard_ptr(_name)(&__unique)) _fail; \
>          else { }
> #define cond_class(_name, _fail, args... ) \
>     __cond_class(__UNIQUE_ID(guard), _name, _fail, args...
> 
> ?
>
Yes, certainly.

Your solution is more elegant. We can reuse __UNIQUE_ID(). There is no need of 
a new helper macro. Thanks.

But with s/cond_class/cond_guard/ and s/guard/scope/ (I think that "scope" 
makes the purpose of that variable clearer).  

I think I'll wait a bit more in case someone else wants to comment and then 
I'll submit v5.

Fabio
> 
> > Jonathan
> > 
> > > Fabio