Add cond_guard() macro to conditional guards. Show how to use it by
converting two functions in drivers/cxl to replace open-coded up_read()
in show_targetN() and in cxl_inject_poison() and avoid to 'goto' to a
cleanup block marked by an 'out' label. The second conversion is added
with v5 to show that the most recent implementation of cond_guard() can
also be used more than once in the same scope.
Cc: Dave Jiang <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Suggested-by: Dan Williams <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
Changes from RFC v5:
Changed the interface of cond_guard() to take a statement or
statement-expression as its second argument to conform to Dan's
suggestion (thanks).
Changes from v1:
Fixed a grammar error in the commit message of 1/2; replaced the
name of the second argument of cond_guard() with '_fail'
according to Jonathan's comments (thanks).
Changes from v2:
Changed macro's implementation to add an 'else' to protect
against it being used incorrectly within another if() block.
Suggested by Dan (thanks). The Reviewed-by tags on 1/2 are not
forwarded because the implementation of cond_guard() has changed.
Removed a redundant 'else' from show_targetN() in 2/2.
Changes from v3:
Added braces around empty body in an 'else' statement in
cond_guard(). Added Reviewed-by tags (Dave, Ira - thanks).
Changes from v4:
Made it possible to call cond_guard() more than once in the same
scope. Jonathan suggested a more elegant implementation than the
one I had thought. It uses an exhisting helper instead of creating
an ad hoc one (thanks). Added a second conversion to show that
cond_guard() can be used twice in the same scope. Deleted the
Reviewed-by tags from 1/3 since the implementation of cond_guard()
has significantly been reworked to allow multiple calls.
Fabio M. De Francesco (3):
cleanup: Add cond_guard() to conditional guards
cxl/region: Use cond_guard() in show_targetN()
cxl/memdev: Use cond_guard() in cxl_inject_poison()
drivers/cxl/core/memdev.c | 19 +++++--------------
drivers/cxl/core/region.c | 16 ++++------------
include/linux/cleanup.h | 20 ++++++++++++++++++++
3 files changed, 29 insertions(+), 26 deletions(-)
--
2.43.2
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. This macro can be called multiple
times in the same scope.
Cc: Dave Jiang <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Suggested-by: Dan Williams <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Suggested-by: Jonathan Cameron <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
include/linux/cleanup.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..602afb85da34 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -134,6 +134,19 @@ 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, fail, args...):
+ * a guard to be used with the conditional variants of locks, like
+ * down_read_trylock() or mutex_lock_interruptible(). 'fail' is a
+ * statement or statement-expression that is executed if waiting for a
+ * lock is interrupted or if a _trylock() fails in case of contention.
+ *
+ * Example:
+ *
+ * cond_guard(mutex_intr, return -EINTR, &mutex);
+ *
+ * This macro can be called multiple times in the same scope, for it
+ * declares unique instances of type 'name'.
+ *
* 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 +178,13 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
#define __guard_ptr(_name) class_##_name##_lock_ptr
+#define __cond_guard(__unique, _name, _fail, args...) \
+ CLASS(_name, __unique)(args); \
+ if (!__guard_ptr(_name)(&__unique)) _fail; \
+ else { }
+#define cond_guard(_name, _fail, args...) \
+ __cond_guard(__UNIQUE_ID(scope), _name, _fail, args)
+
#define scoped_guard(_name, args...) \
for (CLASS(_name, scope)(args), \
*done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
--
2.43.2
Use cond_guard() in show_target() to not open code an up_read() in an 'out'
block. If the down_read_interruptible() fails, the statement passed to the
second argument of cond_guard() returns -EINTR.
Cc: Peter Zijlstra <[email protected]>
Suggested-by: Dan Williams <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
Reviewed-by: Ira Weiny <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
drivers/cxl/core/region.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index ce0e2d82bb2b..704102f75c14 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -666,28 +666,20 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
{
struct cxl_region_params *p = &cxlr->params;
struct cxl_endpoint_decoder *cxled;
- int rc;
- rc = down_read_interruptible(&cxl_region_rwsem);
- if (rc)
- return rc;
+ cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
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");
- else
- rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
- up_read(&cxl_region_rwsem);
+ return sysfs_emit(buf, "\n");
- return rc;
+ return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
}
static int match_free_decoder(struct device *dev, void *data)
--
2.43.2
Use cond_guard() in cxl_inject_poison() to not open code two up_write()
in an 'out' block. If the down_read_interruptible() fail, the statements
passed as the second argument of cond_guard() return -EINTR.
Cc: Dave Jiang <[email protected]>
Cc: Jonathan Cameron <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Suggested-by: Dan Williams <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
drivers/cxl/core/memdev.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index dae8802ecdb0..bd97eea65bb0 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -331,19 +331,13 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
if (!IS_ENABLED(CONFIG_DEBUG_FS))
return 0;
- rc = down_read_interruptible(&cxl_region_rwsem);
- if (rc)
- return rc;
+ cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
- rc = down_read_interruptible(&cxl_dpa_rwsem);
- if (rc) {
- up_read(&cxl_region_rwsem);
- return rc;
- }
+ cond_guard(rwsem_read_intr, return -EINTR, &cxl_dpa_rwsem);
rc = cxl_validate_poison_dpa(cxlmd, dpa);
if (rc)
- goto out;
+ return rc;
inject.address = cpu_to_le64(dpa);
mbox_cmd = (struct cxl_mbox_cmd) {
@@ -353,7 +347,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
};
rc = cxl_internal_send_cmd(mds, &mbox_cmd);
if (rc)
- goto out;
+ return rc;
cxlr = cxl_dpa_to_region(cxlmd, dpa);
if (cxlr)
@@ -366,11 +360,8 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
.length = cpu_to_le32(1),
};
trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_INJECT);
-out:
- up_read(&cxl_dpa_rwsem);
- up_read(&cxl_region_rwsem);
- return rc;
+ return 0;
}
EXPORT_SYMBOL_NS_GPL(cxl_inject_poison, CXL);
--
2.43.2
On Sat, 17 Feb 2024 11:59:02 +0100
"Fabio M. De Francesco" <[email protected]> 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. This macro can be called multiple
> times in the same scope.
>
> Cc: Dave Jiang <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Suggested-by: Dan Williams <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Suggested-by: Jonathan Cameron <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
Looks good.
Reviewed-by: Jonathan Cameron <[email protected]>
On Sat, 17 Feb 2024 11:59:04 +0100
"Fabio M. De Francesco" <[email protected]> wrote:
> Use cond_guard() in cxl_inject_poison() to not open code two up_write()
> in an 'out' block. If the down_read_interruptible() fail, the statements
> passed as the second argument of cond_guard() return -EINTR.
>
> Cc: Dave Jiang <[email protected]>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Suggested-by: Dan Williams <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
On 2/17/24 3:59 AM, 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. This macro can be called multiple
> times in the same scope.
>
> Cc: Dave Jiang <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Suggested-by: Dan Williams <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Suggested-by: Jonathan Cameron <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> include/linux/cleanup.h | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..602afb85da34 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -134,6 +134,19 @@ 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, fail, args...):
> + * a guard to be used with the conditional variants of locks, like
> + * down_read_trylock() or mutex_lock_interruptible(). 'fail' is a
> + * statement or statement-expression that is executed if waiting for a
> + * lock is interrupted or if a _trylock() fails in case of contention.
> + *
> + * Example:
> + *
> + * cond_guard(mutex_intr, return -EINTR, &mutex);
> + *
> + * This macro can be called multiple times in the same scope, for it
> + * declares unique instances of type 'name'.
> + *
> * 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 +178,13 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>
> #define __guard_ptr(_name) class_##_name##_lock_ptr
>
> +#define __cond_guard(__unique, _name, _fail, args...) \
> + CLASS(_name, __unique)(args); \
> + if (!__guard_ptr(_name)(&__unique)) _fail; \
> + else { }
> +#define cond_guard(_name, _fail, args...) \
> + __cond_guard(__UNIQUE_ID(scope), _name, _fail, args)
> +
> #define scoped_guard(_name, args...) \
> for (CLASS(_name, scope)(args), \
> *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
On 2/17/24 3:59 AM, Fabio M. De Francesco wrote:
> Use cond_guard() in cxl_inject_poison() to not open code two up_write()
> in an 'out' block. If the down_read_interruptible() fail, the statements
> passed as the second argument of cond_guard() return -EINTR.
>
> Cc: Dave Jiang <[email protected]>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Suggested-by: Dan Williams <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> drivers/cxl/core/memdev.c | 19 +++++--------------
> 1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index dae8802ecdb0..bd97eea65bb0 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -331,19 +331,13 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
> if (!IS_ENABLED(CONFIG_DEBUG_FS))
> return 0;
>
> - rc = down_read_interruptible(&cxl_region_rwsem);
> - if (rc)
> - return rc;
> + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
>
> - rc = down_read_interruptible(&cxl_dpa_rwsem);
> - if (rc) {
> - up_read(&cxl_region_rwsem);
> - return rc;
> - }
> + cond_guard(rwsem_read_intr, return -EINTR, &cxl_dpa_rwsem);
>
> rc = cxl_validate_poison_dpa(cxlmd, dpa);
> if (rc)
> - goto out;
> + return rc;
>
> inject.address = cpu_to_le64(dpa);
> mbox_cmd = (struct cxl_mbox_cmd) {
> @@ -353,7 +347,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
> };
> rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> if (rc)
> - goto out;
> + return rc;
>
> cxlr = cxl_dpa_to_region(cxlmd, dpa);
> if (cxlr)
> @@ -366,11 +360,8 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
> .length = cpu_to_le32(1),
> };
> trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_INJECT);
> -out:
> - up_read(&cxl_dpa_rwsem);
> - up_read(&cxl_region_rwsem);
>
> - return rc;
> + return 0;
> }
> EXPORT_SYMBOL_NS_GPL(cxl_inject_poison, CXL);
>