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

[Peter Zijlstra <peterz@infradead.org>]

On Wed, May 07, 2025 at 02:18:25PM -0700, Dan Williams wrote:
> Peter Zijlstra wrote:
> [..]
> > > @@ -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...
> 
> I suspect the disconnect is that this proposal adds safety where guard()
> does not today. That was driven by the mistake that Linus caught in the
> RFC [1]
> 
> 	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
> 
> [1]: http://lore.kernel.org/CAHk-=wgRPDGvofj1PU=NemF6iFu308pFZ=w5P+sQyOMGd978fA@mail.gmail.com
> 
> I.e. in my haste I forgot to cleanup a straggling open-coded
> mutex_unlock(), but that is something the compiler warns about iff we
> switch to parallel primitive universe.

AFAICT all you've done is an effective rename. You can still write:

	mutex_unlock(&foo->lock.lock);

and the compiler will again happily do so.

> > What's wrong with:
> > 
> > 	CLASS(mutex_intr, lock)(&foo);
> > 	if (IS_ERR(__guard_ptr(mutex_intr)(lock)))
> > 		return __guard_ptr(mutex_intr)(lock);
> 
> __guard_ptr() returns NULL on error, not an ERR_PTR, but I get the gist.

(Yeah, I realized after sending, but didn't want to self-reply for
something minor like that :-)

> > 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.
> 
> Yes, the "entire set of parallel primitives" was the least confident
> part of this proposal, but the more I look, that is a feature (albeit
> inelegant) not a bug.
> 
> Today one can write:
> 
>     guard(mutex_intr)(&lock);
>     ...
>     mutex_unlock(lock);
> 
> ...and the compiler does not tell you that the lock may not even be held
> upon return, nor that this is unlocking a lock that will also be
> unlocked when @lock goes out of scope.
> 
> The only type safety today is the BUILD_BUG_ON() in scoped_cond_guard()
> when passing in the wrong lock class.
> 
> 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/

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