[RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks

[RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks

[Dan Williams]

The scoped_cond_guard() helper has two problems. First, with "convert to
cleanup" style patches, it results in messy diffs that re-indent large
swaths of code. Second, it swallows return values from interruptible /
killable locks, i.e. the return value of those is not always -EINTR.

The last attempt at improving this situation, if_not_guard() [1], was
reverted with a suggestion:

    "I really think the basic issue is that 'cond_guard' itself is a pretty
     broken concept. It simply doesn't work very well in the C syntax.

     I wish people just gave up on it entirely rather than try to work
     around that fundamental fact."

Take that to heart and abandon a "guard-like" statement. Instead, formalize
an off-the-cuff Linus proposal that was suggested along the way towards
scoped_cond_guard() [2].

    "You should aim for a nice

     struct rw_semaphore *struct rw_semaphore *exec_update_lock
         __cleanup(release_exec_update_lock) = get_exec_update_lock(task);"

Introduce DEFINE_DROP(), to create a cleanup function for releasing locks,
and __drop() to pair with "acquire" helpers. Note, while this could use
DEFINE_FREE()/__free() directly, that leads to confusing a "resource
leak" semantic with "missing unlock". Also, note the name "drop" is a
second option. The first option of calling this semantic "release" collides
with the "__release()" macro already being taken by sparse.

Lastly, just manually define mutex_intr_acquire() and
rwsem_read_intr_acquire() helpers as it was not readily apparent how to
create a new DEFINE_ACQUIRE() macro to automate these definitions. Use the
helpers in at least one location to test out the proposal.

Link: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com [1]
Link: https://lore.kernel.org/all/CAHk-=whvOGL3aNhtps0YksGtzvaob_bvZpbaTcVEqGwNMxB6xg@mail.gmail.com [2]
Cc: David Lechner <dlechner@baylibre.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/mbox.c   |  7 ++++---
 drivers/cxl/core/memdev.c | 18 ++++++++----------
 include/linux/cleanup.h   |  7 +++++++
 include/linux/mutex.h     | 10 ++++++++++
 include/linux/rwsem.h     | 11 +++++++++++
 5 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d72764056ce6..e3e851813c2a 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1394,9 +1394,10 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 	int nr_records = 0;
 	int rc;
 
-	rc = mutex_lock_interruptible(&mds->poison.lock);
-	if (rc)
-		return rc;
+	struct mutex *lock __drop(mutex_unlock) =
+		mutex_intr_acquire(&mds->poison.lock);
+	if (IS_ERR(lock))
+		return PTR_ERR(lock);
 
 	po = mds->poison.list_out;
 	pi.offset = cpu_to_le64(offset);
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index a16a5886d40a..9fbe83f868c6 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -231,15 +231,15 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
 	if (!port || !is_cxl_endpoint(port))
 		return -EINVAL;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
+	struct rw_semaphore *region_rwsem __drop(up_read) =
+		rwsem_read_intr_acquire(&cxl_region_rwsem);
+	if (IS_ERR(region_rwsem))
+		return PTR_ERR(region_rwsem);
 
-	rc = down_read_interruptible(&cxl_dpa_rwsem);
-	if (rc) {
-		up_read(&cxl_region_rwsem);
-		return rc;
-	}
+	struct rw_semaphore *dpa_rwsem __drop(up_read) =
+		rwsem_read_intr_acquire(&cxl_dpa_rwsem);
+	if (IS_ERR(dpa_rwsem))
+		return PTR_ERR(dpa_rwsem);
 
 	if (cxl_num_decoders_committed(port) == 0) {
 		/* No regions mapped to this memdev */
@@ -248,8 +248,6 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
 		/* Regions mapped, collect poison by endpoint */
 		rc =  cxl_get_poison_by_endpoint(port);
 	}
-	up_read(&cxl_dpa_rwsem);
-	up_read(&cxl_region_rwsem);
 
 	return rc;
 }
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 7e57047e1564..3e96edb1b373 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -199,6 +199,13 @@
 
 #define __free(_name)	__cleanup(__free_##_name)
 
+/*
+ * For lock "acquire"/"drop" helpers the lock is not "freed", but the
+ * cleanup mechanics are identical
+ */
+#define DEFINE_DROP(_name, _type, _drop) DEFINE_FREE(_name, _type, _drop)
+#define __drop(_name) __free(_name)
+
 #define __get_and_null(p, nullvalue)   \
 	({                                  \
 		__auto_type __ptr = &(p);   \
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2143d05116be..9fef4077604c 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -202,6 +202,16 @@ DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
 DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
 DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
 
+DEFINE_DROP(mutex_unlock, struct mutex *, if (!IS_ERR_OR_NULL(_T)) mutex_unlock(_T))
+static inline __must_check struct mutex *mutex_intr_acquire(struct mutex *lock)
+{
+	int ret = mutex_lock_interruptible(lock);
+
+	if (ret)
+		return ERR_PTR(ret);
+	return lock;
+}
+
 extern unsigned long mutex_get_owner(struct mutex *lock);
 
 #endif /* __LINUX_MUTEX_H */
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index c8b543d428b0..d1c992b45625 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -242,6 +242,17 @@ DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
 DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
 DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
 
+DEFINE_DROP(up_read, struct rw_semaphore *, if (!IS_ERR_OR_NULL(_T)) up_read(_T))
+static inline __must_check struct rw_semaphore *
+rwsem_read_intr_acquire(struct rw_semaphore *sem)
+{
+	int ret = down_read_interruptible(sem);
+
+	if (ret)
+		return ERR_PTR(ret);
+	return sem;
+}
+
 DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
 DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
 

base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e
-- 
2.49.0

Re: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks

[Linus Torvalds]

"

On Fri, 2 May 2025 at 16:48, Dan Williams <dan.j.williams@intel.com> wrote:
>
> +       struct mutex *lock __drop(mutex_unlock) =
> +               mutex_intr_acquire(&mds->poison.lock);
> +       if (IS_ERR(lock))
> +               return PTR_ERR(lock);

I do think you'd want to have some macro to make this less full of
random boiler plate.

I'd like to point out that when I said that thing you quote:

    "You should aim for a nice

     struct rw_semaphore *struct rw_semaphore *exec_update_lock
         __cleanup(release_exec_update_lock) = get_exec_update_lock(task);"

that comment was fairly early in the whole cleanup.h process, and
"cond_guard()" - which then turned out to not work very well - was
actually a "let's try to clean that up" thing.

And the real issues have been that people who wanted this actually had
truly ugly code that they wanted to mindlessly try to rewrite to use
cleanup. And in the process it actually just got worse.

So I don't think you should take that early off-the-cuff remark of
mine as some kind of solid advice.

I don't know what the best solution is, but it's almost certainly not
this "write out a lot of boiler plate".

Now, the two things that made me suggest that was not that the code
was pretty, but

 (a) having an explicit cleanup syntax that matches the initial assignment

     ie that "__drop(mutex_unlock)" pairs with mutex_intr_acquire()"

and

 (b) having those two things *together* in the same place

where that (a) was to avoid having arbitrary crazy naming that made no
sense ("__free()" of a lock that you didn't allocate, but that you
took? No thank you), and (b) was to avoid having that situation where
you first assign NULL just because you're going to do the locking
elsewhere, and the lock release function is described in a totally
different place from where we acquire it.

So the goal of "cond_guard()" was to have those things together, but
obviously it just then ended up being very messy.

Your patch may end up fixing some of that failure of cond_guard(), but
at the same time I note that your patch is horribly broken. Look at
your change to drivers/cxl/core/mbox.c: you made it use

+       struct mutex *lock __drop(mutex_unlock) =
+               mutex_intr_acquire(&mds->poison.lock);

but then you didn't remove the existing unlocking, so that function still has

        mutex_unlock(&mds->poison.lock);

at the end. So I think the patch shows that it's really easy to just
mess up the conversion, and that this is certainly not a way to "fix"
things.

               Linus

Re: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks

[Dan Williams]

Linus Torvalds wrote:
[..]
> So I don't think you should take that early off-the-cuff remark of
> mine as some kind of solid advice.
> 
> I don't know what the best solution is, but it's almost certainly not
> this "write out a lot of boiler plate".
[..]
> 
> Your patch may end up fixing some of that failure of cond_guard(), but
> at the same time I note that your patch is horribly broken. Look at
> your change to drivers/cxl/core/mbox.c: you made it use
> 
> +       struct mutex *lock __drop(mutex_unlock) =
> +               mutex_intr_acquire(&mds->poison.lock);
> 
> but then you didn't remove the existing unlocking, so that function still has
> 
>         mutex_unlock(&mds->poison.lock);
> 
> at the end. So I think the patch shows that it's really easy to just
> mess up the conversion, and that this is certainly not a way to "fix"
> things.

So, I didn't hear a hard "no" in the above, which lead me to try again
with the incremental changes below, to remove the boilerplate and to
force any conversions to the new scheme to opt-in to a new type such that
you can not make the same mutex_unlock() mistake.

I do note that the original guard() helpers do not protect against that
mistake.

I will stop here though if this is not moving towards viability.

-- 8< --
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index e3e851813c2a..cec9dfb22567 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1394,8 +1394,7 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 	int nr_records = 0;
 	int rc;
 
-	struct mutex *lock __drop(mutex_unlock) =
-		mutex_intr_acquire(&mds->poison.lock);
+	CLASS(mutex_intr_acquire, lock)(&mds->poison.lock);
 	if (IS_ERR(lock))
 		return PTR_ERR(lock);
 
@@ -1431,7 +1430,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		}
 	} while (po->flags & CXL_POISON_FLAG_MORE);
 
-	mutex_unlock(&mds->poison.lock);
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
@@ -1467,7 +1465,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
 		return rc;
 	}
 
-	mutex_init(&mds->poison.lock);
+	mutex_acquire_init(&mds->poison.lock);
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 3ec6b906371b..9b4ab5d1a7c4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -257,7 +257,7 @@ struct cxl_poison_state {
 	u32 max_errors;
 	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
 	struct cxl_mbox_poison_out *list_out;
-	struct mutex lock;  /* Protect reads of poison list */
+	struct mutex_acquire lock;  /* Protect reads of poison list */
 };
 
 /*
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 9fef4077604c..9684f0438364 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -64,6 +64,8 @@ do {									\
 	__mutex_init((mutex), #mutex, &__key);				\
 } while (0)
 
+#define mutex_acquire_init(lock) mutex_init(&(lock)->mutex)
+
 /**
  * mutex_init_with_key - initialize a mutex with a given lockdep key
  * @mutex: the mutex to be initialized
@@ -202,15 +204,23 @@ DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
 DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
 DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
 
-DEFINE_DROP(mutex_unlock, struct mutex *, if (!IS_ERR_OR_NULL(_T)) mutex_unlock(_T))
-static inline __must_check struct mutex *mutex_intr_acquire(struct mutex *lock)
-{
-	int ret = mutex_lock_interruptible(lock);
-
-	if (ret)
-		return ERR_PTR(ret);
-	return lock;
-}
+struct mutex_acquire {
+	struct mutex mutex;
+};
+
+DEFINE_CLASS(mutex_intr_acquire, struct mutex_acquire *,
+		if (!IS_ERR_OR_NULL(_T)) mutex_unlock(&_T->mutex),
+		({
+			struct mutex_acquire *__lock;
+			int __ret = mutex_lock_interruptible(&acq->mutex);
+
+			if (__ret)
+				__lock = ERR_PTR(__ret);
+			else
+				__lock = (struct mutex_acquire *) &acq->mutex;
+			__lock;
+		}),
+		struct mutex_acquire *acq)
 
 extern unsigned long mutex_get_owner(struct mutex *lock);
 

Re: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks

[Linus Torvalds]

On Fri, 2 May 2025 at 19:02, Dan Williams <dan.j.williams@intel.com> wrote:
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index e3e851813c2a..cec9dfb22567 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1394,8 +1394,7 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>         int nr_records = 0;
>         int rc;
>
> -       struct mutex *lock __drop(mutex_unlock) =
> -               mutex_intr_acquire(&mds->poison.lock);
> +       CLASS(mutex_intr_acquire, lock)(&mds->poison.lock);
>         if (IS_ERR(lock))
>                 return PTR_ERR(lock);

Ok, so this whole CLASS-based model looks much better to me and fixes
my concerns.

Let's see if somebody else hates it, but I think this might be viable.

That said, I have to admit that it still looks pretty odd, but it does
look like something people can get used to.

               Linus