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 <[email protected]>
Cc: Dan Williams <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
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
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 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 <[email protected]>
> Cc: Dan Williams <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco
> <[email protected]> ---
>
> 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)
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
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
On Thu, 01 Feb 2024 09:16:59 +0100
"Fabio M. De Francesco" <[email protected]> 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
>
>
>
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" <[email protected]> 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/[email protected]/
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
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" <[email protected]> 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
On Thu, 01 Feb 2024 16:32:25 +0100
"Fabio M. De Francesco" <[email protected]> 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" <[email protected]> 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
>
>
>
>