Received-SPF: pass (google.com: domain of linux-kernel+bounces-45539-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 30 Jan 2024 16:43:04 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Dan Williams <dan.j.williams@intel.com>,
	"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: <65b9979895a8e_5cc6f2942f@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240130164059.25130-1-fabio.maria.de.francesco@linux.intel.com>
 <65b92b91a41a8_5cc6f29484@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <65b9434712b57_2d455e294a8@iweiny-mobl.notmuch>
 <65b948987e865_5cc6f294df@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <65b98e8d4f405_2f26102943c@iweiny-mobl.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <65b98e8d4f405_2f26102943c@iweiny-mobl.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?iFEt3bm2xaNKTPca46yDc4MFOO0g5UKYfRnHtTG0oxcHSFg1esj6nI0C6Y8B?=
 =?us-ascii?Q?X3YA4/fPGDlDRe6AYVmpjSjQ6hI2ViK73SNdY/xKVkPLytW2fNI5bjyBk24S?=
 =?us-ascii?Q?UgH/37NOnDub4qYWCHWor/1d5eR+epj8v4hahiEbQDyPgrNQN/WeDoKdABxt?=
 =?us-ascii?Q?xV7HpNC1a3JWt2GGD6vkcdP22vFMEUfNJ9bXLftIr9GOOUImReNHH4rF0ONo?=
 =?us-ascii?Q?TO9jFazAPUKTXDZqZaNVgW9xqLIwCvCHLwpydxnEmU25Pxw96HBOZQFgabHA?=
 =?us-ascii?Q?QswzjRccFei/WYw7YbGS668ROxeFk0VaTKFh44hxvxyqMAtZvpoEx9Y1HCFs?=
 =?us-ascii?Q?m8zP1dNxdeX8S1B04g5PGo1PriFHIrk3B+5oKgtj24ioVRHRPUQPdSZ0a8/g?=
 =?us-ascii?Q?mtPElPlcihdzbKHwhxT+J/yYIKMNLpT7lsQQ2Nsv1/chVxm6izfh/XHZXoc6?=
 =?us-ascii?Q?JkRXSd7f68+3kn3L2nMrS+Bs9ccH+26H2VTXrEqTWUoZBJelPCX9++BGfYvh?=
 =?us-ascii?Q?77aThCOiluW9d+T0C0DZaBDn8VCYY57KOfndQxY/jde3px+++2m5oxlezUkv?=
 =?us-ascii?Q?luFKTpae6JyGZXHEsZnulkskWvskT+s05mwWtT8rVZiEKQ6kHOyP3d7RFGcx?=
 =?us-ascii?Q?Q1rQU7Z+Y6+npFRdsR4X3Wfn+J31lZqktcY93eKm6gr7Qh/pgp3ze7uJv557?=
 =?us-ascii?Q?Yz/Dp5lSoQYHg13xwwjbjkvwGfaWStyrpp8nVy3cBfAomVlbEcXDQkAiB9zu?=
 =?us-ascii?Q?LudTsziwPfdvByLiTZ9ZsZ+kbvWsLBiVC6RCRZ4Zf7ME0Bya8DLClWa2AlBL?=
 =?us-ascii?Q?1Y467O39lZaylGDqvX+9A7t3p3w3iCMymboPb2LRzbPv/HILL+g+1l5k15/t?=
 =?us-ascii?Q?krwa3UyiCtHOoY5NVL3FdsS5vkI86Qp771G1dlCA7PkSgYoDcQYH8I5VHjVt?=
 =?us-ascii?Q?I7ZWcpeM9wioybo4KEcT5hw7y5BlnH3Uj0ow3Y73hH256DHnTSxGlWz4yB/6?=
 =?us-ascii?Q?eFN5q0tMY9YP7pq6tqHEK7rUJAnthLG2nT10gCHvmNxAO9HGhiz2f5chEzzU?=
 =?us-ascii?Q?5TzTPZkOK7HNp8K/ko9Rbzn83+WeaiSah5jHoA4lJ6GQw0vYUIQwbZGlIYz5?=
 =?us-ascii?Q?l/6hctlSxpvMMTFB+MVENMDiTjMlDyPCMpZWNjGqorUnMMVNfFo3J5UkTihc?=
 =?us-ascii?Q?PHwp6VIGeMKQTEWfMgMYY3obeKVvEci9feOdPZdVgfpcTSyu5EZ6jjdfp8pF?=
 =?us-ascii?Q?Ulj1xiu/kPMoxvKg0DDW6uOUgS8ybc3ydq8PFC1ZW3VLen82ouOwyDt247x5?=
 =?us-ascii?Q?5rI8eoo1FcVSOVX5L0uxUIF6h4aEAmIrCfYRhH2hpEEd4yLpTLXl2okydF/P?=
 =?us-ascii?Q?JycjEXXYki7Nx4TeNM3zjd6jwL0MVx+4sz3yGhCYeHAuAT7PZRjtBtcSf9Z3?=
 =?us-ascii?Q?TiG6XuROky0kfrODZp+wgWOHBbVP7tmGbN+/ZkyvqeLkN78VZc5svYRxPtNW?=
 =?us-ascii?Q?+YSk7d3ycVuEav5jS3Rl83ppVNiaq3bYM2OH/BQ6VO/f0ruL5uekR8CQdPx1?=
 =?us-ascii?Q?kmQuXtx9Ezx7+NqvSg4l0g24bHZ1t2IFC/WUH9pqJ8B/qkDpo81bZMCdb9pp?=
 =?us-ascii?Q?Pg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4518c7cc-5bcb-4f4b-21d2-08dc21f59748
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 00:43:06.9949
 (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: ryJkS4sLLEDLaop+kWGx+AYoIWMgpj7dEFCSRJnVvpK0Jgj4n0/hvvvb1FaA++yRqsf9ShAX/QZsae8so1Cxcsei2rxyTwflVKCPcyBEDSA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4602
X-OriginatorOrg: intel.com

Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> > > Dan Williams wrote:
> > > > 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...
> > > 
> > > I sympathize with the hidden "if" being confusing but there is already
> > > precedent in the current *_guard macros.  So I'd like to know if Peter has
> > > an opinion.
> > 
> > What are you asking specifically? The current scoped_cond_guard()
> > already properly encapsulates the "if ()" and takes an "_fail" so why
> > wouldn't cond_guard() also safely encpsulate an "if ()" and take an
> > "_fail" statement argument?
> 
> Maybe I misunderstood you.  I thought you were advocating that the 'if'
> would not be encapsulated.  And I was wondering if Peter had an opinion.
> 

Last I sent to Fabio was this:

>> 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 */;


> But if you are agreeing with the direction of this patch regarding the if
> then ignore me.

I disagree with the proposal that the caller needs to understand that
the macro leaves a dangling "if ()". I am ok with aligning with
scoped_cond_guard() where the caller can assume that the "_fail"
statement has been executed with no "if ()" sequence to terminate.