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

[Jonathan Cameron]

On Thu, 01 Feb 2024 16:32:25 +0100
"Fabio M. De Francesco" <fabio.maria.de.francesco@xxxxxxxxxxxxxxx> wrote:

> On Thursday, 1 February 2024 16:13:34 CET Fabio M. De Francesco wrote:
> > On Thursday, 1 February 2024 12:36:12 CET Jonathan Cameron wrote:  
> > > On Thu, 01 Feb 2024 09:16:59 +0100
> > > 
> > > "Fabio M. De Francesco" <fabio.maria.de.francesco@xxxxxxxxxxxxxxx> wrote:  
> > > > [snip]
> > > > 
> > > > Actually, I'm doing this:
> > > > 	cond_guard(..., rc, 0, -EINTR, ...);  
> > > 
> > > Can we not works some magic to do.
> > > 
> > > 	cond_guard(..., return -EINTR, ...)
> > > 
> > > and not have an rc at all if we don't want to.
> > > 
> > > Something like
> > > 
> > > #define cond_guard(_name, _fail, args...) \
> > > 
> > > 	CLASS(_name, scope)(args); \
> > > 	if (!__guard_ptr(_name)(&scope)) _fail
> > > 
> > > Completely untested so I'm probably missing some subtleties.
> > > 
> > > Jonathan  
> > 
> > Jonathan,
> > 
> > Can you please comment on the v5 of this RFC?

Would lose context of this discussion.

> > It is at
> > https://lore.kernel.org/all/20240201131033.9850-1-fabio.maria.de.francesco@
> > linux.intel.com/
> > 
> > The macro introduced in v5 has the following, more general, use case:
> > 
> > * * 	int ret;
> > + * 	// down_read_trylock() returns 1 on success, 0 on contention
> > + * 	cond_guard(rwsem_read_try, ret, 1, 0, &sem);
> > + * 	if (!ret) {
> > + * 		dev_dbg("down_read_trylock() failed to down 'sem')\n");
> > + * 		return ret;
> > + * 	}
> > 
> > The text above has been copy-pasted from the RFC Patch v5.
> > 
> > Please notice that we need to provide both the success and the failure code
> > to make it work also with the _trylock() variants (more details in the
> > patch).  
> 
> The next three lines have been messed up by a copy-paste.
> They are:
> 
> If we simply do something like:
> 
> 	cond_guard(..., ret = 0, ...)
> 
> We won't store the success (that is 1) in ret and it would still contain 0, 
> that is the code of the contended case.

 
If there are cases that need to do different things in the two paths the
define full conditions for success and failure.

#define cond_guard(_name, _fail, _success, args...) \
 	CLASS(_name, scope)(args); \
 	if (!__guard_ptr(_name)(&scope)) _fail; \
	else _success

However I'm not sure that additional complexity is worth while.
Maybe just handling failure is all we need.

This should allow

cond_guard(rwsem_read_try, return -EINVAL, , lock); or
cond_guard(rwsem_read_try, rc = 1, rc = 0, lock);

So similar to scoped_cond_guard() there is no need to
have a local variable if all you want to do is return on
failure.

> 
> > If we simply do something like:
> > 
> > 	cond_guard(..., ret = 0, ...)
> > 
> > to be able store in 'ret' the code of the contended case, that is 0.
> > 
> > Since down_read_trylock() returns 1 on down semaphore, when we later check
> > 'ret' with "if (!ret) <failure path>;" we always enter in that failure path
> > even if the semaphore is down because we didn't store the success code in
> > ret (and ret is still probably 0).
> > 
> > This is why, I think, we need a five arguments cond_guard(). This can manage
> > also the _interruptible() and _killable() cases as:
> > 
> > 	cond_guard(..., ret, 0, -EINTR, ...)
> > 
> > In this case we don't need 5 arguments, but we have a general use case, one
> > only macro, that can work with all the three variants of locks.
> > 
> > Fabio  
> 
> 
> 
>