Received-SPF: pass (google.com: domain of linux-kernel+bounces-48369-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: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Peter Zijlstra <peterz@infradead.org>, dan.j.williams@intel.com,
 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 16:13:34 +0100
Message-ID: <2172852.irdbgypaU6@fdefranc-mobl3>
Organization: intel
In-Reply-To: <20240201113612.00001d90@Huawei.com>
References:
 <20240131134108.423258-1-fabio.maria.de.francesco@linux.intel.com>
 <1785013.VLH7GnMWUR@fdefranc-mobl3> <20240201113612.00001d90@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

On Thursday, 1 February 2024 12:36:12 CET Jonathan Cameron wrote:
> On Thu, 01 Feb 2024 09:16:59 +0100
> 
> "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com> wrote:
> >
> > [snip]
> > 
> > Actually, I'm doing this:
> > 	cond_guard(..., rc, 0, -EINTR, ...);
> 
> Can we not works some magic to do.
> 	cond_guard(..., return -EINTR, ...)
> 
> and not have an rc at all if we don't want to.
> 
> Something like
> 
> #define cond_guard(_name, _fail, args...) \
> 	CLASS(_name, scope)(args); \
> 	if (!__guard_ptr(_name)(&scope)) _fail
> 
> Completely untested so I'm probably missing some subtleties.
> 
> Jonathan
> 
Jonathan,

Can you please comment on the v5 of this RFC?
It is at https://lore.kernel.org/all/20240201131033.9850-1-fabio.maria.de.francesco@linux.intel.com/

The macro introduced in v5 has the following, more general, use case:

* * 	int ret;
+ * 	// down_read_trylock() returns 1 on success, 0 on contention
+ * 	cond_guard(rwsem_read_try, ret, 1, 0, &sem);
+ * 	if (!ret) {
+ * 		dev_dbg("down_read_trylock() failed to down 'sem')\n");
+ * 		return ret;
+ * 	}

The text above has been copy-pasted from the RFC Patch v5.

Please notice that we need to provide both the success and the failure code to 
make it work also with the _trylock() variants (more details in the patch).

If we simply do something like:
	
	cond_guard(..., ret = 0, ...)

to be able store in 'ret' the code of the contended case, that is 0.

Since down_read_trylock() returns 1 on down semaphore, when we later check 
'ret' with "if (!ret) <failure path>;" we always enter in that failure path 
even if the semaphore is down because we didn't store the success code in ret 
(and ret is still probably 0).

This is why, I think, we need a five arguments cond_guard(). This can manage 
also the _interruptible() and _killable() cases as:

	cond_guard(..., ret, 0, -EINTR, ...) 

In this case we don't need 5 arguments, but we have a general use case, one 
only macro, that can work with all the three variants of locks.

Fabio