Re: [PATCH 1/7] cleanup: Introduce DEFINE_ACQUIRE() a CLASS() for conditional locking

[Dan Williams <dan.j.williams@intel.com>]

Peter Zijlstra wrote:
[..]
> > So the proposal is, if you know what you are doing, or have a need to
> > switch back and forth between scope-based and explicit unlock for a give
> > lock, use the base primitives. If instead you want to fully convert to
> > scope-based lock management (excise all explicit unlock() calls) *and*
> > you want the compiler to validate the conversion, switch to the _acquire
> > parallel universe.
> 
> As with all refactoring ever, the rename trick always works. But I don't
> think that warrants building a parallel infrastructure just for that.
> 
> Specifically, it very much does not disallow calling mutex_unlock() on
> your new member. So all you get is some compiler help during refactor,
> and again, just rename the lock member already.
> 
> Also, if we ever actually get LLVM's Thread Safety Analysis working,
> that will help us with all these problems:
> 
>   https://lore.kernel.org/all/20250304092417.2873893-1-elver@google.com/

That looks lovely.

> But the compiler needs a little more work go grok C :-)

Ok, here is a last shot that incorporates all the feedback:

1/ Conceptually no need for a new CLASS() save for the fact that
   __guard_ptr() returns NULL on failure, not an ERR_PTR().

2/ The rename trick is not true type safety, especially if it leads to
   parallel universe of primitives, but it is a useful trick.

3/ "IS_ERR(__guard_ptr(mutex_intr)(lock))" is a mouthful, would be nice
   to have something more succint while maintaining some safety.

That leads me to a scheme like the following:

    DEFINE_GUARD_ERR(mutex, _intr, mutex_lock_interruptible(_T))
    ...
    ACQUIRE(mutex_intr, lock)(&obj->lock);
    if (IS_ERR(lock))
       return PTR_ERR(lock);

Where that guard class differs from mutex_intr in that it returns an
ERR_PTR(). Some type-safety is provided by ACQUIRE() which looks for the
"mutex_intr_err" class not "mutex_intr".

The rename trick can be opt-in for helping to validate refactoring, but
the expectation is that something like Thread Safety Analysis should
make that rename track moot. In the meantime code stays with all the
same named primitives, only ever one primitive universe to handle.

I am open to making the rule be that "#define MUTEX_ACQUIRE" never ships
upstream, it's only a local hack during code refactoring to check
assumptions.

-- 8< --
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d72764056ce6..b767d3e8f9c7 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1,9 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
+#define MUTEX_ACQUIRE
+#include <linux/mutex.h>
 #include <linux/security.h>
 #include <linux/debugfs.h>
 #include <linux/ktime.h>
-#include <linux/mutex.h>
 #include <linux/unaligned.h>
 #include <cxlpci.h>
 #include <cxlmem.h>
@@ -1394,9 +1395,9 @@ 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;
+	ACQUIRE(mutex_intr, lock)(&mds->poison.lock);
+	if (IS_ERR(lock))
+		return PTR_ERR(lock);
 
 	po = mds->poison.list_out;
 	pi.offset = cpu_to_le64(offset);
@@ -1430,7 +1431,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");
@@ -1466,7 +1466,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
 		return rc;
 	}
 
-	mutex_init(&mds->poison.lock);
+	mutex_init(&mds->poison.lock.mutex);
 	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/cleanup.h b/include/linux/cleanup.h
index 7e57047e1564..403947d2537e 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -291,16 +291,21 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 #define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
 static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 
+#define __DEFINE_CLASS_IS_ERR_PTR(_name, _is_err)	\
+static __maybe_unused const bool class_##_name##_is_err_ptr = _is_err
+
 #define __DEFINE_GUARD_LOCK_PTR(_name, _exp) \
 	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
 	{ return (void *)(__force unsigned long)*(_exp); }
 
-#define DEFINE_CLASS_IS_GUARD(_name) \
+#define DEFINE_CLASS_IS_GUARD(_name)                 \
 	__DEFINE_CLASS_IS_CONDITIONAL(_name, false); \
+	__DEFINE_CLASS_IS_ERR_PTR(_name, false);     \
 	__DEFINE_GUARD_LOCK_PTR(_name, _T)
 
-#define DEFINE_CLASS_IS_COND_GUARD(_name) \
+#define DEFINE_CLASS_IS_COND_GUARD(_name)           \
 	__DEFINE_CLASS_IS_CONDITIONAL(_name, true); \
+	__DEFINE_CLASS_IS_ERR_PTR(_name, false);    \
 	__DEFINE_GUARD_LOCK_PTR(_name, _T)
 
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
@@ -309,6 +314,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 
 #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
 	__DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true); \
+	__DEFINE_CLASS_IS_ERR_PTR(_name##_ext, false); \
 	EXTEND_CLASS(_name, _ext, \
 		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
 		     class_##_name##_t _T) \
@@ -320,6 +326,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 #define __is_cond_ptr(_name) class_##_name##_is_conditional
+#define __is_guard_err_ptr(_name) class_##_name##_is_err_ptr
 
 /*
  * Helper macro for scoped_guard().
@@ -346,6 +353,7 @@ _label:									\
 	for (CLASS(_name, scope)(args); true; ({ goto _label; }))	\
 		if (!__guard_ptr(_name)(&scope)) {			\
 			BUILD_BUG_ON(!__is_cond_ptr(_name));		\
+			BUILD_BUG_ON(__is_guard_err_ptr(_name));	\
 			_fail;						\
 _label:									\
 			break;						\
@@ -424,5 +432,43 @@ __DEFINE_LOCK_GUARD_0(_name, _lock)
 	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
 	{ return class_##_name##_lock_ptr(_T); }
 
+#define EXTEND_CLASS_ERR(_name, ext, _init, _init_args...)                    \
+	typedef class_##_name##_t class_##_name##ext##_err_t;                 \
+	static inline void class_##_name##ext##_err_destructor(               \
+		class_##_name##_t *p)                                         \
+	{                                                                     \
+		/* base destructors do not expect ERR_PTR */                  \
+		if (IS_ERR(p))                                                \
+			p = NULL;                                             \
+		class_##_name##_destructor(p);                                \
+	}                                                                     \
+	static inline class_##_name##_t class_##_name##ext##_err_constructor( \
+		_init_args)                                                   \
+	{                                                                     \
+		class_##_name##_t t = _init;                                  \
+		return t;                                                     \
+	}
+
+#define DEFINE_GUARD_ERR(_name, _ext, _condlock)                \
+	__DEFINE_CLASS_IS_CONDITIONAL(_name##_ext##_err, true); \
+	__DEFINE_CLASS_IS_ERR_PTR(_name##_ext##_err, true);     \
+	EXTEND_CLASS_ERR(_name, _ext, ({                        \
+				 void *_t = _T;                 \
+				 int err = _condlock;           \
+                                                                \
+				 if (err)                       \
+					 _t = ERR_PTR(err);     \
+				 _t;                            \
+			 }),                                    \
+			 class_##_name##_t _T)                  \
+	static inline void *class_##_name##_ext##_err_lock_ptr( \
+		class_##_name##_t *_T)                          \
+	{                                                       \
+		return class_##_name##_lock_ptr(_T);            \
+	}
+
+#define ACQUIRE(_name, var)                                                   \
+	class_##_name##_err_t var __cleanup(class_##_name##_err_destructor) = \
+		class_##_name##_err_constructor
 
 #endif /* _LINUX_CLEANUP_H */
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2143d05116be..bd4b449ea6bd 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -198,9 +198,37 @@ extern void mutex_unlock(struct mutex *lock);
 
 extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
 
+/*
+ * For subsystems that want to use a "rename trick" to use type-safety
+ * to validate debug scope-based unlock, define MUTEX_ACQUIRE before
+ * including mutex.h.
+ */
+struct mutex_acquire {
+	/* private: */
+	struct mutex mutex;
+};
+
+static inline int mutex_try_or_busy(struct mutex *lock)
+{
+	int ret[] = { -EBUSY, 0 };
+
+	return ret[mutex_trylock(lock)];
+}
+
+#ifndef MUTEX_ACQUIRE
 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_GUARD_ERR(mutex, _intr, mutex_lock_interruptible(_T))
+DEFINE_GUARD_ERR(mutex, _try, mutex_try_or_busy(_T))
+#else
+DEFINE_GUARD(mutex, struct mutex_acquire *, mutex_lock(&_T->mutex),
+	     mutex_unlock(&_T->mutex))
+DEFINE_GUARD_COND(mutex, _try, mutex_trylock(&_T->mutex))
+DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(&_T->mutex) == 0)
+DEFINE_GUARD_ERR(mutex, _intr, mutex_lock_interruptible(&_T->mutex))
+DEFINE_GUARD_ERR(mutex, _try, mutex_try_or_busy(&_T->mutex))
+#endif
 
 extern unsigned long mutex_get_owner(struct mutex *lock);