Le 12/03/2024 à 00:47, George Stark a écrit : > [Vous ne recevez pas souvent de courriers de gnstark@xxxxxxxxxxxxxxxxx. > Découvrez pourquoi ceci est important à > https://aka.ms/LearnAboutSenderIdentification ] > > Hello Waiman, Marek > > Thanks for the review. > > I've never used lockdep for debug but it seems preferable to > keep that feature working. It could be look like this: For sure it is a must. I'm not used to it either hence my overlook. > > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index f7611c092db7..574f6de6084d 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,31 @@ do > { \ > > #ifdef CONFIG_DEBUG_MUTEXES > > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock); > + > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + int ret; \ > + mutex_init(mutex); \ > + ret = debug_devm_mutex_init(dev, mutex); \ > + ret; \ > +}) > + I think it would be preferable to minimise the number of macros. If I were you I would keep your devm_mutex_init() as is but rename it __devm_mutex_init() and just remove the mutex_init() from it, then add only one macro that works independant of CONFIG_DEBUG_MUTEXES: #define devm_mutex_init(dev, mutex) \ ({ \ mutex_init(mutex); \ __devm_mutex_init(dev, mutex); \ }) With that, no need of a second version of the macro and no need for the typecheck either. Note the __ which is a clear indication that allthough that function is declared in public mutex.h, it is not meant to be used outside of it. > void mutex_destroy(struct mutex *lock); > > #else > > +/* > +* When CONFIG_DEBUG_MUTEXES is off mutex_destroy is just a nop so > +* there's no really need to register it in devm subsystem. > +*/ > +#define devm_mutex_init(dev, mutex) \ > +({ \ > + typecheck(struct device *, dev); \ > + mutex_init(mutex); \ > + 0; \ > +}) > + > static inline void mutex_destroy(struct mutex *lock) {} > > #endif > diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c > index bc8abb8549d2..967a5367c79a 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" > > @@ -89,6 +90,16 @@ void debug_mutex_init(struct mutex *lock, const char > *name, > lock->magic = lock; > } > > +static void devm_mutex_release(void *res) > +{ > + mutex_destroy(res); > +} > + > +int debug_devm_mutex_init(struct device *dev, struct mutex *lock) > +{ > + return devm_add_action_or_reset(dev, devm_mutex_release, lock); > +} > + > /*** > * mutex_destroy - mark a mutex unusable > * @lock: the mutex to be destroyed > -- > 2.25.1 > > > > And now I would drop the the refactoring patch with moving down > mutex_destroy. devm block is big enough to be declared standalone. > > > On 3/7/24 19:44, Marek Behún wrote: >> 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-debug.c >>>>> 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 > > -- > Best regards > George
