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

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

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.

> 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.

> 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.

> 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.

I think that is solvable, but only with a new DEFINE_LOCK_GUARD_1() that
knows that the @lock member of class_##name##_t needs to be cast to the
base lock type.