Re: [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init

[Marek Behún]

On Thu, 7 Mar 2024 08:39:46 -0500
Waiman Long <longman@xxxxxxxxxx> wrote:

> On 3/7/24 04:56, Marek Behún wrote:
> > On Thu, Mar 07, 2024 at 05:40:26AM +0300, George Stark wrote:  
> >> Using of devm API leads to a certain order of releasing resources.
> >> So all dependent resources which are not devm-wrapped should be deleted
> >> with respect to devm-release order. Mutex is one of such objects that
> >> often is bound to other resources and has no own devm wrapping.
> >> Since mutex_destroy() actually does nothing in non-debug builds
> >> frequently calling mutex_destroy() is just ignored which is safe for now
> >> but wrong formally and can lead to a problem if mutex_destroy() will be
> >> extended so introduce devm_mutex_init()
> >>
> >> Signed-off-by: George Stark <gnstark@xxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> >> ---
> >>   Hello Christophe. Hope you don't mind I put you SoB tag because you helped alot
> >>   to make this patch happen.
> >>
> >>   include/linux/mutex.h        | 13 +++++++++++++
> >>   kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
> >>   2 files changed, 35 insertions(+)
> >>
> >> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> >> index f7611c092db7..9bcf72cb941a 100644
> >> --- a/include/linux/mutex.h
> >> +++ b/include/linux/mutex.h
> >> @@ -22,6 +22,8 @@
> >>   #include <linux/cleanup.h>
> >>   #include <linux/mutex_types.h>
> >>
> >> +struct device;
> >> +
> >>   #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >>   # define __DEP_MAP_MUTEX_INITIALIZER(lockname)			\
> >>   		, .dep_map = {					\
> >> @@ -115,10 +117,21 @@ do {							\
> >>
> >>   #ifdef CONFIG_DEBUG_MUTEXES
> >>
> >> +int devm_mutex_init(struct device *dev, struct mutex *lock);
> >>   void mutex_destroy(struct mutex *lock);
> >>
> >>   #else
> >>
> >> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> >> +{
> >> +	/*
> >> +	 * since mutex_destroy is nop actually there's no need to register it
> >> +	 * in devm subsystem.
> >> +	 */
> >> +	mutex_init(lock);
> >> +	return 0;
> >> +}
> >> +
> >>   static inline void mutex_destroy(struct mutex *lock) {}
> >>
> >>   #endif
> >> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debugc
> >> index bc8abb8549d2..c9efab1a8026 100644
> >> --- a/kernel/locking/mutex-debug.c
> >> +++ b/kernel/locking/mutex-debug.c
> >> @@ -19,6 +19,7 @@
> >>   #include <linux/kallsyms.h>
> >>   #include <linux/interrupt.h>
> >>   #include <linux/debug_locks.h>
> >> +#include <linux/device.h>
> >>
> >>   #include "mutex.h"
> >>
> >> @@ -104,3 +105,24 @@ void mutex_destroy(struct mutex *lock)
> >>   }
> >>
> >>   EXPORT_SYMBOL_GPL(mutex_destroy);
> >> +
> >> +static void devm_mutex_release(void *res)
> >> +{
> >> +	mutex_destroy(res);
> >> +}
> >> +
> >> +/**
> >> + * devm_mutex_init - Resource-managed mutex initialization
> >> + * @dev:	Device which lifetime mutex is bound to
> >> + * @lock:	Pointer to a mutex
> >> + *
> >> + * Initialize mutex which is automatically destroyed when the driver is detached.
> >> + *
> >> + * Returns: 0 on success or a negative error code on failure.
> >> + */
> >> +int devm_mutex_init(struct device *dev, struct mutex *lock)
> >> +{
> >> +	mutex_init(lock);
> >> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> >> +}
> >> +EXPORT_SYMBOL_GPL(devm_mutex_init);  
> > Hi George,
> >
> > look at
> > https://lore.kernel.org/lkml/7013bf9e-2663-4613-ae61-61872e81355b@xxxxxxxxxx/
> > where Matthew and Hans explain that devm_mutex_init needs to be a macro
> > because of the static lockdep key.
> >
> > so this should be something like:
> >
> > static inline int __devm_mutex_init(struct device *dev, struct mutex *mutex,
> > 				    const char *name,
> > 				    struct lock_class_key *key)
> > {
> > 	__mutex_init(mutex, name, key);
> > 	return devm_add_action_or_reset(dev, devm_mutex_release, mutex);
> > }
> >
> > #define devm_mutex_init(dev, mutex)				\
> > do {								\
> > 	static struct lock_class_key __key;			\
> > 								\
> > 	__devm_mutex_init(dev, (mutex), #mutex, &__key);	\
> > } while (0);
> >
> >
> > Marek  
> 
> Making devm_mutex_init() a function will make all the devm_mutex share 
> the same lockdep key. Making it a macro will make each caller of 
> devm_mutex_init() have a distinct lockdep key. It all depends on whether 
> all the devm_mutexes have the same lock usage pattern or not and whether 
> it is possible for one devm_mutex to be nested inside another. So either 
> way can be fine depending on the mutex usage pattern. My suggestion is 
> to use a function, if possible, unless it will cause a false positive 
> lockdep splat as there is a limit on the maximum # of lockdep keys that 
> can be used.

devm_mutex_init() should behave like other similar function
initializing stuff with resource management. I.e. it should behave like
mutex_init(), but with resource management.

mutex_init() is a macro generating static lockdep key for each instance,
so devm_mutex_init() should also generate static lockdep key for each
instance.

Marek