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

[Peter Zijlstra <peterz@infradead.org>]

On Wed, May 07, 2025 at 12:21:39AM -0700, Dan Williams wrote:
> 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."
> 
> However, the observation is that just exposing the CLASS() to callers
> solves: the safety concern (compiler enforced "expected expression"
> checks), conveying the conditional acquisition state of the lock, and
> avoiding re-indentation caused by scoped_cond_guard(). See the commit below
> for the analysis of the revert.
> 
> Commit b4d83c8323b0 ("headers/cleanup.h: Remove the if_not_guard() facility")
> 
> The DEFINE_ACQUIRE() macro wraps a lock type like 'struct mutex' into a
> 'struct mutex_acquire' type that can only be acquired and automatically
> released by scope-based helpers. E.g.:
> 
>     [scoped_]guard(mutex_acquire)(...)
>     CLASS(mutex_intr_acquire, ...)
>     CLASS(mutex_try_acquire, ...)
> 
> Use DEFINE_ACQUIRE to create the new classes above and use
> mutex_intr_acquire in the CXL subsystem to demonstrate the conversion.
> 

> +#define DEFINE_ACQUIRE(_name, _locktype, _unlock, _cond_lock)                \
> +	DEFINE_CLASS(_name, struct _locktype##_acquire *,                    \
> +		     if (!IS_ERR_OR_NULL(_T)) _unlock(&_T->_locktype), ({    \
> +			     struct _locktype##_acquire *lock_result;        \
> +			     int ret = _cond_lock(&to_lock->_locktype);      \
> +                                                                             \
> +			     if (ret)                                        \
> +				     lock_result = ERR_PTR(ret);             \
> +			     else                                            \
> +				     lock_result =                           \
> +					     (struct _locktype##_acquire     \
> +						      *)&to_lock->_locktype; \
> +			     lock_result;                                    \
> +		     }),                                                     \
> +		     struct _locktype##_acquire *to_lock)
>  
>  #endif /* _LINUX_CLEANUP_H */
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index 2143d05116be..283111f43b0f 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,6 +204,28 @@ 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)
>  
> +/* mutex type that only implements scope-based unlock */
> +struct mutex_acquire {
> +	/* private: */
> +	struct mutex mutex;
> +};
> +DEFINE_GUARD(mutex_acquire, struct mutex_acquire *, mutex_lock(&_T->mutex),
> +	     mutex_unlock(&_T->mutex))
> +DEFINE_GUARD_COND(mutex_acquire, _try, mutex_trylock(&_T->mutex))
> +DEFINE_GUARD_COND(mutex_acquire, _intr, mutex_lock_interruptible(&_T->mutex) == 0)
> +DEFINE_ACQUIRE(mutex_intr_acquire, mutex, mutex_unlock,
> +	       mutex_lock_interruptible)
> +
> +static inline int mutex_try_or_busy(struct mutex *lock)
> +{
> +	int ret[] = { -EBUSY, 0 };
> +
> +	return ret[mutex_trylock(lock)];
> +}
> +
> +DEFINE_ACQUIRE(mutex_try_acquire, mutex, mutex_unlock,
> +	       mutex_try_or_busy)
> +
>  extern unsigned long mutex_get_owner(struct mutex *lock);
>  
>  #endif /* __LINUX_MUTEX_H */

I'm terribly confused...

What's wrong with:

	CLASS(mutex_intr, lock)(&foo);
	if (IS_ERR(__guard_ptr(mutex_intr)(lock)))
		return __guard_ptr(mutex_intr)(lock);

I mean, yeah __guard_ptr(mutex_intr) doesn't really roll of the tongue,
but if that is the whole objection, surely we can try and fix that bit
instead of building an entire parallel set of primitives.


Notably, you're going to be running into trouble the moment you want to
use your acquire stuff on things like raw_spin_trylock_irqsave(),
because all that already wraps the return type, in order to hide the
flags thing etc.