Received-SPF: pass (google.com: domain of linux-kernel+bounces-45510-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 30 Jan 2024 16:04:29 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Dan Williams <dan.j.williams@intel.com>, Ira Weiny <ira.weiny@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: <65b98e8d4f405_2f26102943c@iweiny-mobl.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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <65b948987e865_5cc6f294df@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?iweVt5j9RQ/0zkj9hICMAM+VK6WslYJcUwWCA4849nls7IdaZlqFTzaVNT4N?=
 =?us-ascii?Q?0Sk4HE8TJ1g3Cz1s3CWoNF9nlGvLuW7IRoMikw8f/+1tzx+nFhOV8L3SP+2t?=
 =?us-ascii?Q?Z8T1P3Iez9BaNg5tgc8kRbegqeAfDWVL9bpz7amtcS1zHPvNso7MYFU0qgb7?=
 =?us-ascii?Q?lpIjrnW33HQs7E4rmS243WqDCOPQr68/3WrU1qaYqlX0NF48MAlAVdjCjtyE?=
 =?us-ascii?Q?1NGliFqVwBOALqHys4Au8mNKPy0/0yHKtA6mXS36c1AHhdINFpAo1D0aeRbz?=
 =?us-ascii?Q?Pbrcvr5E/FMV+RzEuov4KLaVfpZd5Ug3K4IZLPN+0fbUsGqcBLfL7ze2RBjR?=
 =?us-ascii?Q?G5r2R9M5TLyu3DWWdClJMfPn1YVDq2J/8kxePNvrUABwIQo3yV+ykD4+KcuU?=
 =?us-ascii?Q?WuQFmvJJRtdO9u2SvrEvvL+JYzlGtBznvzUqLsRYBs04Tm3Kj3bD7RazY1Lv?=
 =?us-ascii?Q?365OxaBBKvvsd/4AIt9K2LWcVhCL0ffGD2r3ADgGxntH03yp5a+0VULAdXbj?=
 =?us-ascii?Q?ZR+EoQhiWpIk0THnFg/ue0kmbqT23PM1Eh4K4R8Gv38I0wez6HsE+AS+n4B3?=
 =?us-ascii?Q?vNvITbwPK5cZIORhA60/iGqrB2UdZkPAUARYLpHz14jjOKlJRF5dSsefUG2T?=
 =?us-ascii?Q?BlrYbctlQ3klc8B6kuOhqPSMWS/mlY+wewsvbqAfwNzMMXx6E1f4bl5pO6Ts?=
 =?us-ascii?Q?WVZZOVaxIHRBd4eBaJSFh33HTzDivyWTYnEHgtBj53MhSIBn1hG0ucahbxCK?=
 =?us-ascii?Q?gRgJSQeFwL7A6dT5IHU7B+ebz9RySkR7YTT5T8Wto2TblO7XxcyQHKQ9PqGs?=
 =?us-ascii?Q?iQv9ZThh2nYrPUBSWNt339QBNluXZlmI0ogK5H9Qz7AsOLHnkM/SCV/3D7MC?=
 =?us-ascii?Q?YbZUXcKWEpX+m3DFW5z8MJ+SGMt06L/b7u/6JDgRKnEJq63vgY14bz6NbMz1?=
 =?us-ascii?Q?3Rl0ztoK3zq0Isb0Qv8rQWgYNa7lIrH6sC2EqjJhfaxUlmFw7ooL3BPnv/ZC?=
 =?us-ascii?Q?bocoN65hz+OaIAtEAE9OdwbichhDkRDo2yDNc+y+dkGcOqAE/PHLQpgoGUZP?=
 =?us-ascii?Q?fd02AxVBYXM9oQaVQKqOjfgYtiaNNmwWfwO9fxQBd9KpQymVpq9C37pLt0SS?=
 =?us-ascii?Q?GKFb5pHoXG/Rc9rYll4aj+9SDPdswmjfr6KUmn1cBM5cshT2xnaQ0AVk2Oov?=
 =?us-ascii?Q?9D6FwKpd/dhFva38Xka8XYyOXkoeb6yhWEhwq6Cqqun/jQZRiaUTsvXeYkSN?=
 =?us-ascii?Q?x2Q8bBTAGh/3Fv4HmVs1lzDkTeic0RlgOSBghH0KL6WGxYJ9s5AnHKPxumY+?=
 =?us-ascii?Q?4CXWDe/05shf5b9z5JJp18TwcS1VLDq1rJH/XI+x8EV1j9NGXBG0pU+of7r+?=
 =?us-ascii?Q?Vpk5fsjRIkUBB2v/cE70xiJ0QiaHG+EWnqTU+RhRO3PVfgv2CaIivSRk3GcW?=
 =?us-ascii?Q?UY00YR3pQhdDLf/VUvGQG5PF3D2GJDwWf45SXzQxegTZZGG4cpU6rHeVzkBs?=
 =?us-ascii?Q?zxh5VMbT54jzQ+NbCOFodosVp8WnsuFNshB1J/VKsEabXCxxAoAH2mfUgE8H?=
 =?us-ascii?Q?BUWhUp8wyum8Ija2Z0C0PBG0iRwSfVNaR34mhOy6?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 96329e5c-2fd4-4500-0b4b-08dc21f03422
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 00:04:33.2184
 (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: OkSt/FD8dUsLOqRKyeYPKwXigmfxu8VULFySPe6HsC7WFVq/KzOz/9bBylBmQm5Fjp+SRTpgIhLkqTOik1HzQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6614
X-OriginatorOrg: intel.com

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.

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

Ira