Received-SPF: pass (google.com: domain of linux-kernel+bounces-45008-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 30 Jan 2024 09:02:09 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>,
	<linux-kernel@vger.kernel.org>
CC: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>, "Peter
 Zijlstra" <peterz@infradead.org>, Dan Williams <dan.j.williams@intel.com>,
	Ira Weiny <ira.weiny@intel.com>
Subject: Re: [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards
Message-ID: <65b92b91a41a8_5cc6f29484@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240130164059.25130-1-fabio.maria.de.francesco@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240130164059.25130-1-fabio.maria.de.francesco@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?7AV5/DXOawTg9STkFBuntRVG3/eQdJx9eZRmSAe89BYCyzknGerqjkDDS6k2?=
 =?us-ascii?Q?mpDnzAKNM7XWO8Zt9tpEDPBpi/gOrXnHlXIDz5TeRlKdf5+82oUuMLUr9ynu?=
 =?us-ascii?Q?8CwG+O3Yh+J5FTIipasLiYOXX7pdsy+U8Jdkn6HmlPV+v1Cdq+OMuWSHoo4d?=
 =?us-ascii?Q?Glfol6R1IP21zEfV7nKr9SvHU0MWmdZEEHuUuAwxZltrWvEC+ohKs7RcTfC1?=
 =?us-ascii?Q?DENZuVyoid3p+IT+ugWOsVtH1IHtv+Y0jZgYHuGGZGBjxCE+JDzPqSPFlKi5?=
 =?us-ascii?Q?4EAad4BYjs/70f6hLLjrRH2JPRUILYrsLijWhLjsSrXFogS2oMv+f67J60vM?=
 =?us-ascii?Q?4rgX0aHLhMyAJobD1aIK8QI/3uGC48m+0CQYOQNoZ5Kt0npGuSgjAOy72iRn?=
 =?us-ascii?Q?zVlVAWp7YNkLim6KrrcYSddESjigQEe1/mHCEYBVOS5r4oYR9syHcD8xqLY2?=
 =?us-ascii?Q?kHqgHQBXLxl+FFv1CJHHBl3TqEYVxZfd4zi+TfD9gq+T8kEMB6nXs2VNpqKY?=
 =?us-ascii?Q?rrOJFRX6dhExNI3XDdjSNjaECFiP2+ej2f9u0OdZHjCjp0sjqpBWdEdojSFZ?=
 =?us-ascii?Q?OjTROwQ3TvIQ8/b8IbFdpG4W614rKKWxu4lhdN+5dNOeCpXAz+L2iVIry+sa?=
 =?us-ascii?Q?UCsgQnQEiGIqBOZK3A6HzBjlPK/3R8Qk5+jnmcqSHFxZJltK40LWG2FQU+2m?=
 =?us-ascii?Q?MoL68rvke314LeY5I8Us5TwVmzK8G8XnDiueRGdVUgyyBH4+tw7cXd74yxcY?=
 =?us-ascii?Q?QAgrCKSDwzItAirCsVROe1HGCBvKm0fhBqjDOUK4y2if68LSl0w3/pT8XNat?=
 =?us-ascii?Q?3LM8OFDZ4hVZFazypE59qF8zt9Uke04cC79XygLOTqcybFyygeUz7MxzGYQk?=
 =?us-ascii?Q?mvgS6nqSS7y+6DSmdV3LMdPwPQ0at41M2KI6vOKbj94gf0WYT3kj8YBlZxTU?=
 =?us-ascii?Q?lx8ccEPWfjv66hyvg47AqcZ8TPspAGj91YHd4hxJNBNZToXv6yrcjrkg+Hdi?=
 =?us-ascii?Q?TWSVAWlh07Kx7ZW+VhVEbRxtWA4Usdk2LDFYqpaFZ7p+PmecnkWfTduSQTPB?=
 =?us-ascii?Q?9gLUO79K9ZNpkFdXuPqWmqeZ4/BM+Sb0LrDPtyaBv5ClTlRfUPY1a4pVtQ1C?=
 =?us-ascii?Q?tXeLy394bzGGuHVm4nkgUT+Cmd98smKeS2LEpG7jgcQad5PVhdTA4Nx0ZrN7?=
 =?us-ascii?Q?Y2B92kjfC8g46RuuWjknz6mKrm9n5EkXjcn11Ov/uMdM/WWeW8bKI3qFtOcL?=
 =?us-ascii?Q?xSJi3cOJzSsgfvYWEc2C3R1fyMrfgMZveWJAxbpV0PsW53iQZzhFXcHmr/Pm?=
 =?us-ascii?Q?j9hgdu2vwHW7oiN5upHCeJfNwZ5IvdUPPwf548F+49VF1zjoxjjBcoE9//6a?=
 =?us-ascii?Q?2/1R0dmh4jNtNDKZQCuBKttAlVmJlI5N4aB8koA08hvK64g9hqSNYmSX+Bb2?=
 =?us-ascii?Q?OffiNKPOtlnrF/laJ9N+CGq4rQlPpEkuPMIScFF5+AydZ9HzukzjStkvJReM?=
 =?us-ascii?Q?75IGn+wCDdSkbirsbWXKJC4JE7IuiVhaZGeWCamP5kCIdIXSq9W1VmIWe7jr?=
 =?us-ascii?Q?Es0LR9mq3h8RpjLESyPDxvXrbQ6LGarWNig8P3LZAs1pMvf6tFBuEPSPRKbc?=
 =?us-ascii?Q?cQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 95902eda-fa3d-4353-5498-08dc21b5337f
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2024 17:02:11.7819
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fccRNHyVz/QOVRgDcoqam5G0onVpQNX1gTSbWTqBekiNcLta5/vZr6fBkFDjGjMO6qCyxlx1xTFUEFAHxuV/vF5c/EgE3NrElpB+jq0C2+A=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB6843
X-OriginatorOrg: intel.com

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 expects a block where the failure can be handled
> (e.g., calling printk() and returning -EINTR 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 remains an RFC because Dan suggested a slightly different syntax:
> 
> 	if (cond_guard(...))
> 		return -EINTR;
> 
> But the scoped_cond_guard() macro omits the if statement:
> 
>     	scoped_cond_guard (...) {
>     	}
> 
> Thus define cond_guard() similarly to scoped_cond_guard() but with a block
> to handle the failure case:
> 
> 	cond_guard(...)
> 		return -EINTR;

That's too subtle for me, because of the mistakes that can be made with
brackets how about a syntax like:

 	cond_guard(..., return -EINTR, ...)

..to make it clear what happens if the lock acquisition fails without
having to remember there is a hidden incomplete "if ()" statement in
that macro? More below...

> 
> 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 | 17 +++++------------
>  include/linux/cleanup.h   | 13 +++++++++++++
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 0f05692bfec3..20d405f01df5 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -666,28 +666,21 @@ 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, &cxl_region_rwsem)
> +		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..fc850e61a47e 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -134,6 +134,15 @@ 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, args...):
> + * 	for conditional locks like mutex_trylock() or down_read_interruptible().
> + * 	It expects a block for handling errors like in the following example:
> + *
> + * 	cond_guard(rwsem_read_intr, &my_sem) {
> + * 		printk(KERN_NOTICE "Try failure in work0()\n");
> + * 		return -EINTR;
> + * 	}
> + *
>   * 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 +174,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
>  
> +#define cond_guard(_name, args...) \
> +	CLASS(_name, scope)(args); \
> +	if (!__guard_ptr(_name)(&scope))

This needs to protect against being used within another if () block.
Imagine a case of:

    if (...) {
    	cond_guard(...);
	    <statement>
    } else if (...)

..does that "else if" belong to the first "if ()" or the hidden one
inside the macro?

You can steal the embedded "if ()" trick from scoped_cond_guard() and do
something like (untested):

#define cond_guard(_name, _fail, args...) \
	CLASS(_name, scope)(args); \
	if (!__guard_ptr(_name)(&scope)) _fail; else /* pass */;