[RFC] cleanup: Add cond_guard() to conditional guards
Commit Message
Add cond_guard() to conditional guards.
cond_guard() stores the return value from _interruptible(), _killable(),
and _try versions of locks to a variable whose address is passed as the
second parameter, so that the value can later be checked and an action
be taken accordingly (e.g., calling printk() in case of failure).
As the other guards, it avoids to open code the release of the lock
after a goto to an 'out' label.
This is an RFC at least for two reasons...
1) I'm not sure that this guard is needed unless we want to avoid the
use of scoped_cond_guard() with its necessary indentation of the code
in the successful path and/or we want to check the return value of the
conditional lock.
2) By using cond_guard() it is not possible to release the lock before
calling other functions from the current one. The lock is held until the
current function exits. This is not always desirable.
The changes for cxl/region are only added to show a possible use case for
cond_guard().
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>
---
drivers/cxl/core/region.c | 7 ++-----
include/linux/cleanup.h | 9 +++++++++
2 files changed, 11 insertions(+), 5 deletions(-)
Comments
Fabio M. De Francesco wrote:
> Add cond_guard() to conditional guards.
>
> cond_guard() stores the return value from _interruptible(), _killable(),
> and _try versions of locks to a variable whose address is passed as the
> second parameter, so that the value can later be checked and an action
> be taken accordingly (e.g., calling printk() in case of failure).
This does not look like it returns the result from the conditional
locking function, it looks like it returns whether the boolean of
whether the CLASS() constructor succeeded or failed:
"*_ret = (!__guard_ptr(_name)(&scope)"
Even if it can't return the result from the lock call itself directly,
it would still be nice to be able to write something like:
rc = cond_guard(rwsem_read_intr, -EINTR, &rwsem);
if (rc)
return rc;
..and know that the lock is taken of -EINTR is returned.
That would also mean being able to write "if (cond_guard())", which I
don't think would work with the current proposal.
> As the other guards, it avoids to open code the release of the lock
> after a goto to an 'out' label.
>
> This is an RFC at least for two reasons...
>
> 1) I'm not sure that this guard is needed unless we want to avoid the
> use of scoped_cond_guard() with its necessary indentation of the code
> in the successful path and/or we want to check the return value of the
> conditional lock.
Taking this argument to its logical conclusion would mean removing plain
guard() as well. There is definitely a code readability benefit for
having the option of guard() vs scoped_guard(), so it would be nice to
have similar flexibility in the conditional case.
> 2) By using cond_guard() it is not possible to release the lock before
> calling other functions from the current one. The lock is held until the
> current function exits. This is not always desirable.
No, that's not correct, the lock is only held until the exit from the
current scope, and if you call functions from within that scope the lock
remains held. So:
func1()
{
guard(...)(lock1);
if (condition) {
guard(...)(lock2);
func2();
}
func3();
}
func2() is called with lock1 and lock2 held, func3() is only called with
lock1() held.
On Monday, 29 January 2024 23:23:17 CET Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > 2) By using cond_guard() it is not possible to release the lock before
> > calling other functions from the current one. The lock is held until the
> > current function exits. This is not always desirable.
>
> No, that's not correct, the lock is only held until the exit from the
> current scope, and if you call functions from within that scope the lock
> remains held. So:
>
> func1()
> {
> guard(...)(lock1);
> if (condition) {
> guard(...)(lock2);
> func2();
> }
> func3();
> }
>
> func2() is called with lock1 and lock2 held, func3() is only called with
> lock1() held.
Dan,
If I read your example correctly, this is exactly what I wanted to say by
"it's not possible to release the lock before calling other functions from the
current one".
For example, if we use this (not scoped) cond_guard() in cxl_region_probe(),
we end up to the switch() and then possibly call devm_cxl_add_pmem_region()
with the cxl_region_rwsem semaphore down, whereas the current code calls a
up_read() before the switch.
I think that in cxl_region_probe() the only suited conditional guard is
scoped_cond_guard().
I know that you don't want an indented success path but in cxl_region_probe
but I suspect that scoped_cond_guard() is the only viable solution, otherwise
we end up calling the functions that now are called after up_read() with the
semaphore still down.
I hope that this time I have been clearer.
Thanks,
Fabio
@@ -668,15 +668,14 @@ 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;
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];
@@ -684,8 +683,6 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
rc = sysfs_emit(buf, "\n");
else
rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
- up_read(&cxl_region_rwsem);
return rc;
}
@@ -134,6 +134,11 @@ 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, _retp, args...):
+ * this assigns *_retp with the return values from conditional locks like
+ * down_read_trylock() or mutex_lock_interruptible(). *_retp can then be
+ * checked and actions can be taken accordingly.
+ *
* 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 +170,10 @@ 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); \
+ *_ret = (!__guard_ptr(_name)(&scope))
+
#define scoped_guard(_name, args...) \
for (CLASS(_name, scope)(args), \
*done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)