Received-SPF: pass (google.com: domain of linux-kernel+bounces-46468-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
To: linux-kernel@vger.kernel.org, Dan Williams <dan.j.williams@intel.com>,
 Dan Williams <dan.j.williams@intel.com>
Cc: 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
Date: Wed, 31 Jan 2024 14:11:09 +0100
Message-ID: <3335653.aeNJFYEL58@fdefranc-mobl3>
Organization: intel
In-Reply-To:
 <65b938c1ad435_5cc6f294eb@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References:
 <20240130164059.25130-1-fabio.maria.de.francesco@linux.intel.com>
 <22169722.EfDdHjke4D@fdefranc-mobl3>
 <65b938c1ad435_5cc6f294eb@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

On Tuesday, 30 January 2024 18:58:25 CET Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > On Tuesday, 30 January 2024 18:02:09 CET 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...
> > 
> > As you propose I can't see how to handle multi-line error path like in:
> > 	cond_guard(...) {
> > 	
> > 		dev_dbg(...);
> > 		return -EINTR;
> > 	
> > 	}
> 
> The _fail argument is a statement, to make it a compound statement maybe
> just add braces, something like:
> 
>     cond_guard(..., { dev_dbg(...); return -EINTR; }, ...)
> 
> ...another possibility is something like
> 
>     int rc = 0;
> 
>     cond_guard(..., rc = -EINTR, ...)
>     if (rc) {
>         ...
>         return rc;
>     }

I had tried this before sending this patch. It looked the most obvious 
solution. But it fails my tests: it always return -EINTR, regardless of the 
successful down.

It looks like it was not expanded as I was expecting.

Or my tests are wrong, but I can't see any obvious mistake.

BTW, it's interesting to notice that the following instead works. I guess that 
it is due to the same fact that required me to pass a pointer to 'rc' in the 
first version of this patch to (mistakenly) store the boolean of whether the 
constructor succeeded or failed.

	int rc;
	int *rcp = &rc;

	cond_guard(..., *rcp = -EINTR, ...)
	if (rc) {
		dev_dbg(...);
		return rc;
	}

This works but I think nobody wants to see anything like this.

Fabio
 
> 
> ...so, I don't think we need separate macros for the multi-statement
> case.