Received-SPF: pass (google.com: domain of linux-kernel+bounces-47478-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
To: Peter Zijlstra <peterz@infradead.org>, dan.j.williams@intel.com
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
 linux-cxl@vger.kernel.org, Ira Weiny <ira.weiny@intel.com>
Subject: Re: [RFC PATCH v3] cleanup: Add cond_guard() to conditional guards
Date: Thu, 01 Feb 2024 02:08:44 +0100
Message-ID: <6168759.DvuYhMxLoT@fdefranc-mobl3>
Organization: intel
In-Reply-To:
 <20240131134108.423258-1-fabio.maria.de.francesco@linux.intel.com>
References: <20240131134108.423258-1-fabio.maria.de.francesco@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

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)