| Message ID | 20240131134108.423258-1-fabio.maria.de.francesco@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v3] cleanup: Add cond_guard() to conditional guards | expand |
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)
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.
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" <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 > > >
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
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
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 > > > >
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)
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(-)