Add cond_guard() macro to conditional guards and use it to replace an
open-coded up_read() in show_targetN() and remove a block marked by an
'out' label.
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]>
---
Changes from RFC v4:
Changed the interface of cond_guard() to take a variable to store
a return code, the succes code and failure code, to enable a
later check of the returned code in that variable.
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.
Fabio M. De Francesco (2):
cleanup: Add cond_guard() to conditional guards
cxl/region: Use cond_guard() in show_targetN()
drivers/cxl/core/region.c | 16 ++++------------
include/linux/cleanup.h | 15 +++++++++++++++
2 files changed, 19 insertions(+), 12 deletions(-)
--
2.43.0
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.
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]>
---
include/linux/cleanup.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..d70454e9f8dc 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -134,6 +134,16 @@ 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);
+ *
* 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 +175,11 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
#define __guard_ptr(_name) class_##_name##_lock_ptr
+#define cond_guard(_name, _fail, args...) \
+ CLASS(_name, scope)(args); \
+ if (!__guard_ptr(_name)(&scope)) _fail; \
+ else
+
#define scoped_guard(_name, args...) \
for (CLASS(_name, scope)(args), \
*done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
--
2.43.0
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: Dave Jiang <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Suggested-by: Dan Williams <[email protected]>
Suggested-by: Ira Weiny <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
Reviewed-by: Ira Weiny <[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 0f05692bfec3..eb5c36462c0a 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.0
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.
>
> 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]>
Thanks for the revisions!
Reviewed-by: Ira Weiny <[email protected]>
On 2/6/24 5:13 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.
>
> 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]>
> ---
> include/linux/cleanup.h | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..d70454e9f8dc 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -134,6 +134,16 @@ 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);
> + *
> * 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 +175,11 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>
> #define __guard_ptr(_name) class_##_name##_lock_ptr
>
> +#define cond_guard(_name, _fail, args...) \
> + CLASS(_name, scope)(args); \
> + if (!__guard_ptr(_name)(&scope)) _fail; \
> + else
> +
> #define scoped_guard(_name, args...) \
> for (CLASS(_name, scope)(args), \
> *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
On 2/6/24 5:13 AM, Fabio M. De Francesco wrote:
> 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: Dave Jiang <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Suggested-by: Dan Williams <[email protected]>
> Suggested-by: Ira Weiny <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> Reviewed-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
Reviewed-by: Dave Jiang <[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 0f05692bfec3..eb5c36462c0a 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)
Hi Fabio,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.8-rc3 next-20240206]
[cannot apply to cxl/next cxl/pending]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cleanup-Add-cond_guard-to-conditional-guards/20240206-203110
base: linus/master
patch link: https://lore.kernel.org/r/20240206121301.7225-3-fabio.maria.de.francesco%40linux.intel.com
patch subject: [PATCH 2/2 v3] cxl/region: Use cond_guard() in show_targetN()
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240207/[email protected]/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240207/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/cxl/core/region.c: In function 'show_targetN':
>> drivers/cxl/core/region.c:670:70: warning: suggest braces around empty body in an 'else' statement [-Wempty-body]
670 | cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
| ^
vim +/else +670 drivers/cxl/core/region.c
664
665 static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
666 {
667 struct cxl_region_params *p = &cxlr->params;
668 struct cxl_endpoint_decoder *cxled;
669
> 670 cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
671
672 if (pos >= p->interleave_ways) {
673 dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
674 p->interleave_ways);
675 return -ENXIO;
676 }
677
678 cxled = p->targets[pos];
679 if (!cxled)
680 return sysfs_emit(buf, "\n");
681
682 return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
683 }
684
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wednesday, 7 February 2024 02:54:34 CET kernel test robot wrote:
> Hi Fabio,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.8-rc3 next-20240206]
> [cannot apply to cxl/next cxl/pending]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cleanup
> -Add-cond_guard-to-conditional-guards/20240206-203110 base: linus/master
> patch link:
> https://lore.kernel.org/r/20240206121301.7225-3-fabio.maria.de.francesco%40
> linux.intel.com patch subject: [PATCH 2/2 v3] cxl/region: Use cond_guard()
> in show_targetN() config: s390-allyesconfig
> (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp
> @intel.com/config) compiler: s390-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build):
> (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp
> @intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of the same patch/commit), kindly add following tags
>
> | Reported-by: kernel test robot <[email protected]>
> | Closes:
> | https://lore.kernel.org/oe-kbuild-all/[email protected]
> | /
> All warnings (new ones prefixed by >>):
>
> drivers/cxl/core/region.c: In function 'show_targetN':
> >> drivers/cxl/core/region.c:670:70: warning: suggest braces around empty
> >> body in an 'else' statement [-Wempty-body]
> 670 | cond_guard(rwsem_read_intr, return -EINTR,
> &cxl_region_rwsem);
> |
^
I think that this warning deserves attention and braces should be added around
the 'else' empty body. I'm going to send v4:
#define cond_guard(_name, _ret, args...) \
CLASS(_name, scope)(args); \
if (!__guard_ptr(_name)(&scope)) _ret; \
else { }
Any comments?
Fabio
>
> vim +/else +670 drivers/cxl/core/region.c
>
> 664
> 665 static size_t show_targetN(struct cxl_region *cxlr, char *buf, int
> pos) 666 {
> 667 struct cxl_region_params *p = &cxlr->params;
> 668 struct cxl_endpoint_decoder *cxled;
> 669
>
> > 670 cond_guard(rwsem_read_intr, return -EINTR,
&cxl_region_rwsem);
>
> 671
> 672 if (pos >= p->interleave_ways) {
> 673 dev_dbg(&cxlr->dev, "position %d out of range
%d\n", pos,
> 674 p->interleave_ways);
> 675 return -ENXIO;
> 676 }
> 677
> 678 cxled = p->targets[pos];
> 679 if (!cxled)
> 680 return sysfs_emit(buf, "\n");
> 681
> 682 return sysfs_emit(buf, "%s\n", dev_name(&cxled-
>cxld.dev));
> 683 }
> 684
Fabio M. De Francesco wrote:
> On Wednesday, 7 February 2024 02:54:34 CET kernel test robot wrote:
> > Hi Fabio,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on linus/master]
> > [also build test WARNING on v6.8-rc3 next-20240206]
> > [cannot apply to cxl/next cxl/pending]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:
> > https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cleanup
> > -Add-cond_guard-to-conditional-guards/20240206-203110 base: linus/master
> > patch link:
> > https://lore.kernel.org/r/20240206121301.7225-3-fabio.maria.de.francesco%40
> > linux.intel.com patch subject: [PATCH 2/2 v3] cxl/region: Use cond_guard()
> > in show_targetN() config: s390-allyesconfig
> > (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp
> > @intel.com/config) compiler: s390-linux-gcc (GCC) 13.2.0
> > reproduce (this is a W=1 build):
> > (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp
> > @intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version
> > of the same patch/commit), kindly add following tags
> >
> > | Reported-by: kernel test robot <[email protected]>
> > | Closes:
> > | https://lore.kernel.org/oe-kbuild-all/[email protected]
> > | /
> > All warnings (new ones prefixed by >>):
> >
> > drivers/cxl/core/region.c: In function 'show_targetN':
> > >> drivers/cxl/core/region.c:670:70: warning: suggest braces around empty
> > >> body in an 'else' statement [-Wempty-body]
> > 670 | cond_guard(rwsem_read_intr, return -EINTR,
> > &cxl_region_rwsem);
> > |
> ^
>
> I think that this warning deserves attention and braces should be added around
> the 'else' empty body. I'm going to send v4:
>
> #define cond_guard(_name, _ret, args...) \
> CLASS(_name, scope)(args); \
> if (!__guard_ptr(_name)(&scope)) _ret; \
> else { }
>
I think this is a good addition. If the user forgets ';' at the end of
the cond_guard() we could have a hidden side effect similar to what Dan
was concerned about... This guarantees that won't happen.
Score one for the bots!
Ira