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

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

[Fabio M. De Francesco]

Add cond_guard() to conditional guards.

cond_guard() is used for the _interruptible(), _killable(), and _try
versions of locks.

It stores a return value to a variable whose address is given to its
second argument, that is either '-1' on failure or '0' on success to
acquire a lock. The returned value can be checked to act accordingly
(e.g., to call printk() and return -EINTR in case of failure of an
_interruptible() variant)

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.

The changes to the CXL code are provided only to show the use of this
macro. If consensus is gathered on this macro, the cleanup of
show_targetN() will be submitted later in a separate patch.

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

Changes from v1 and v2:
	I've taken into account Dan's comments (thanks).

 drivers/cxl/core/region.c | 15 +++++----------
 include/linux/cleanup.h   | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 0f05692bfec3..560f25bdfd11 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -668,26 +668,21 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 	struct cxl_endpoint_decoder *cxled;
 	int rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
+	cond_guard(rwsem_read_intr, &rc, &cxl_region_rwsem);
 	if (rc)
-		return rc;
+		return -EINTR;
 
 	if (pos >= p->interleave_ways) {
 		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
 			p->interleave_ways);
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
 	cxled = p->targets[pos];
 	if (!cxled)
-		rc = sysfs_emit(buf, "\n");
+		return sysfs_emit(buf, "\n");
 	else
-		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+		return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
 }
 
 static int match_free_decoder(struct device *dev, void *data)
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..a4b40d511f9e 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -134,6 +134,20 @@ 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, ret, args...):
+ * 	for conditional locks like mutex_trylock() or down_read_interruptible().
+ * 	'ret' is a pointer to a variable where this macro stores 0 on success
+ * 	and -1 on failure to acquire a lock.
+ *
+ * 	Example:
+ *
+ * 	int ret;
+ * 	cond_guard(rwsem_read_try, &ret, &sem);
+ * 	if (ret) {
+ * 		dev_dbg("down_read_trylock() failed to down 'sem')\n");
+ * 		return 0; // down_read_trylock() returns 0 on contention
+ * 	}
+ *
  * 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 +179,11 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
+#define cond_guard(_name, _ret, args...) \
+	CLASS(_name, scope)(args); \
+	if (!__guard_ptr(_name)(&scope)) *_ret = -1 \
+	else *_ret = 0
+
 #define scoped_guard(_name, args...)					\
 	for (CLASS(_name, scope)(args),					\
 	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
-- 
2.43.0

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

[Fabio M. De Francesco]

I just noticed that this is not the final version. It misses a semicolon. 
Please discard this v3. I'm sending v4.

Fabio

On Wednesday, 31 January 2024 14:37:35 CET 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 stores a return value to a variable whose address is given to its
> second argument, that is either '-1' on failure or '0' on success to
> acquire a lock. The returned value can be checked to act accordingly
> (e.g., to call printk() and return -EINTR in case of failure of an
> _interruptible() variant)
> 
> 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.
> 
> The changes to the CXL code are provided only to show the use of this
> macro. If consensus is gathered on this macro, the cleanup of
> show_targetN() will be submitted later in a separate patch.
> 
> 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> ---
> 
> Changes from v1 and v2:
> 	I've taken into account Dan's comments (thanks).
> 
>  drivers/cxl/core/region.c | 15 +++++----------
>  include/linux/cleanup.h   | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 0f05692bfec3..560f25bdfd11 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -668,26 +668,21 @@ static size_t show_targetN(struct cxl_region *cxlr,
> char *buf, int pos) struct cxl_endpoint_decoder *cxled;
>  	int rc;
> 
> -	rc = down_read_interruptible(&cxl_region_rwsem);
> +	cond_guard(rwsem_read_intr, &rc, &cxl_region_rwsem);
>  	if (rc)
> -		return rc;
> +		return -EINTR;
> 
>  	if (pos >= p->interleave_ways) {
>  		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
>  			p->interleave_ways);
> -		rc = -ENXIO;
> -		goto out;
> +		return -ENXIO;
>  	}
> 
>  	cxled = p->targets[pos];
>  	if (!cxled)
> -		rc = sysfs_emit(buf, "\n");
> +		return sysfs_emit(buf, "\n");
>  	else
> -		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
> -out:
> -	up_read(&cxl_region_rwsem);
> -
> -	return rc;
> +		return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
>  }
> 
>  static int match_free_decoder(struct device *dev, void *data)
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..a4b40d511f9e 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -134,6 +134,20 @@ 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, ret, args...):
> + * 	for conditional locks like mutex_trylock() or
> down_read_interruptible(). + * 	'ret' is a pointer to a variable 
where this
> macro stores 0 on success + * 	and -1 on failure to acquire a lock.
> + *
> + * 	Example:
> + *
> + * 	int ret;
> + * 	cond_guard(rwsem_read_try, &ret, &sem);
> + * 	if (ret) {
> + * 		dev_dbg("down_read_trylock() failed to down 'sem')\n");
> + * 		return 0; // down_read_trylock() returns 0 on contention
> + * 	}
> + *
>   * 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 +179,11 @@ static inline class_##_name##_t
> class_##_name##ext##_constructor(_init_args) \
> 
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
> 
> +#define cond_guard(_name, _ret, args...) \
> +	CLASS(_name, scope)(args); \
> +	if (!__guard_ptr(_name)(&scope)) *_ret = -1 \
> +	else *_ret = 0
> +
>  #define scoped_guard(_name, args...)					\
>  	for (CLASS(_name, scope)(args),					\
>  	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void 
*)1)

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

[Dan Williams]

Fabio M. De Francesco wrote:
> I just noticed that this is not the final version. It misses a semicolon. 
> Please discard this v3. I'm sending v4.

Ok, but do please copy the aspect of scoped_conf_guard() to take a
"_fail" statement argument. Passing a return code collector variable by
reference just feels a bit too magical. I like the explicitness of
passing the statement directly.

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

[Fabio M. De Francesco]

On Thursday, 1 February 2024 02:12:12 CET Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > I just noticed that this is not the final version. It misses a semicolon.
> > Please discard this v3. I'm sending v4.
> 
> Ok, but do please copy the aspect of scoped_conf_guard() to take a
> "_fail" statement argument. Passing a return code collector variable by
> reference just feels a bit too magical. I like the explicitness of
> passing the statement directly.

I'm sorry I haven't been clear. The following call convention fails my tests:

	cond_guard(..., rc = -EINTR, ...);

It always returns -EINTR, regardless of the success of 
down_read_interuptible(). There must be a reason that I can't see.

It works only if we immediaely return an error code: 

	cond_guard(..., return -EINTR, ...);

But this is not what we want since we want to check 'rc'.

Fabio

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

[Fabio M. De Francesco]

On Thursday, 1 February 2024 02:12:12 CET Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > I just noticed that this is not the final version. It misses a semicolon.
> > Please discard this v3. I'm sending v4.
> 
> Ok, but do please copy the aspect of scoped_conf_guard() to take a
> "_fail" statement argument. Passing a return code collector variable by
> reference just feels a bit too magical. I like the explicitness of
> passing the statement directly.

I had introduced a bug in my tests that made me see failures when there were 
not. Now I fixed it and tests don't fail.

I'm sending a new version that passes the return variable directly, not as a 
reference, similar but not equal to:

	cond_guard(..., rc, -EINTR, ...);

Actually, I'm doing this:

	cond_guard(..., rc, 0, -EINTR, ...);

I'm not passing 'rc = -EINTR' because I want to take into account the 
possibility that rc contains values different than 0 from previous assignments. 
I'm passing rc, so that the macro can assign either a success code or a 
failure error to this variable. Any value from previous assignments must be 
always overwritten: 

	#define cond_guard(_name, _ret, _scs, _err, args...) \
        	CLASS(_name, scope)(args); \
        	if (!__guard_ptr(_name)(&scope)) _ret = _err; \
        	else _ret = _scs;

I should have seen long ago that my tests were failing because of a missing 
'else' when passing a statement in 'cond_guard(..., rc = -EINTR, ...);'. It 
had nothing to do with how to pass 'rc'. Sorry for that confusion.

Fabio

Fabio 

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

[Jonathan Cameron]

On Thu, 01 Feb 2024 09:16:59 +0100
"Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com> wrote:

> On Thursday, 1 February 2024 02:12:12 CET Dan Williams wrote:
> > Fabio M. De Francesco wrote:  
> > > I just noticed that this is not the final version. It misses a semicolon.
> > > Please discard this v3. I'm sending v4.  
> > 
> > Ok, but do please copy the aspect of scoped_conf_guard() to take a
> > "_fail" statement argument. Passing a return code collector variable by
> > reference just feels a bit too magical. I like the explicitness of
> > passing the statement directly.  
> 
> I had introduced a bug in my tests that made me see failures when there were 
> not. Now I fixed it and tests don't fail.
> 
> I'm sending a new version that passes the return variable directly, not as a 
> reference, similar but not equal to:
> 
> 	cond_guard(..., rc, -EINTR, ...);
> 
> 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


> 
> I'm not passing 'rc = -EINTR' because I want to take into account the 
> possibility that rc contains values different than 0 from previous assignments. 
> I'm passing rc, so that the macro can assign either a success code or a 
> failure error to this variable. Any value from previous assignments must be 
> always overwritten: 
> 
> 	#define cond_guard(_name, _ret, _scs, _err, args...) \
>         	CLASS(_name, scope)(args); \
>         	if (!__guard_ptr(_name)(&scope)) _ret = _err; \
>         	else _ret = _scs;
> 
> I should have seen long ago that my tests were failing because of a missing 
> 'else' when passing a statement in 'cond_guard(..., rc = -EINTR, ...);'. It 
> had nothing to do with how to pass 'rc'. Sorry for that confusion.
> 
> Fabio
> 
> Fabio 
> 
> 
> 

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

[Fabio M. De Francesco]

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@linux.intel.com> 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?
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).

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 

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

[Fabio M. De Francesco]

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@linux.intel.com> 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?
> 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 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

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@linux.intel.com> 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@linux.intel.com> 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  
> 
> 
> 
>