Messages in this thread | | | From | "Fabio M. De Francesco" <> | Subject | Re: [PATCH 1/2 v4] cleanup: Add cond_guard() to conditional guards | Date | Thu, 15 Feb 2024 14:12:31 +0100 |
| |
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@Huawei.com> wrote: > > On Tue, 13 Feb 2024 17:51:26 +0100 > > > > "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com> 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
| |