Re: [PATCH 2/2] mutex: use spin_[un]lock instead ofarch_spin_[un]lock

[Steven Rostedt]

On Thu, 2013-01-24 at 16:14 -0800, Andrew Morton wrote:
> On Thu, 24 Jan 2013 17:22:45 +0800
> Yuanhan Liu <yuanhan.liu@xxxxxxxxxxxxxxx> wrote:
> 
> > Use spin_[un]lock instead of arch_spin_[un]lock in mutex-debug.h so
> > that we can collect the lock statistics of spin_lock_mutex from
> > /proc/lock_stat.
> > 
> > ...
> >
> > --- a/kernel/mutex-debug.h
> > +++ b/kernel/mutex-debug.h
> > @@ -43,13 +43,13 @@ static inline void mutex_clear_owner(struct mutex *lock)
> >  							\
> >  		DEBUG_LOCKS_WARN_ON(in_interrupt());	\
> >  		local_irq_save(flags);			\
> > -		arch_spin_lock(&(lock)->rlock.raw_lock);\
> > +		spin_lock(lock);			\
> >  		DEBUG_LOCKS_WARN_ON(l->magic != l);	\
> >  	} while (0)
> >  
> >  #define spin_unlock_mutex(lock, flags)				\
> >  	do {							\
> > -		arch_spin_unlock(&(lock)->rlock.raw_lock);	\
> > +		spin_unlock(lock);				\
> >  		local_irq_restore(flags);			\
> >  		preempt_check_resched();			\
> >  	} while (0)
> 
> >From my reading of the c2f21ce2e3128 changelog, this patch might screw
> up the -rt kernel:
> 
>     locking: Implement new raw_spinlock
>     
>     Now that the raw_spin name space is freed up, we can implement
>     raw_spinlock and the related functions which are used to annotate the
>     locks which are not converted to sleeping spinlocks in preempt-rt.
>     
>     A side effect is that only such locks can be used with the low level
>     lock fsunctions which circumvent lockdep.
>     
>     For !rt spin_* functions are mapped to the raw_spin* implementations.

Actually this code is never called when we enable -rt.

At the top of -rt's mutex.h header:

#ifdef CONFIG_PREEMPT_RT_FULL
# include <linux/mutex_rt.h>
#else

[...]

Which basically includes the rest of the mutex.h file.

The kernel/Makefile has:

ifneq ($(CONFIG_PREEMPT_RT_FULL),y)
obj-y += mutex.o
obj-$(CONFIG_DEBUG_MUTEXES) += mutex-debug.o
obj-y += rwsem.o
endif


That's because the -rt kernel uses the priority inheritance rtmutex.
Pretty much the same one that userspace futexes use.

These changes are fine and wont hurt -rt. But thanks for think about
us :-)



> 
> 
> Also, I believe your patch permits this cleanup:
> 
> --- a/kernel/mutex-debug.h~mutex-use-spin_lock-instead-of-arch_spin_lock-fix
> +++ a/kernel/mutex-debug.h
> @@ -42,14 +42,12 @@ static inline void mutex_clear_owner(str
>  		struct mutex *l = container_of(lock, struct mutex, wait_lock); \
>  							\
>  		DEBUG_LOCKS_WARN_ON(in_interrupt());	\
> -		local_irq_save(flags);			\
> -		spin_lock(lock);			\
> +		spin_lock_irqsave(lock, flags);		\
>  		DEBUG_LOCKS_WARN_ON(l->magic != l);	\
>  	} while (0)
>  
>  #define spin_unlock_mutex(lock, flags)				\
>  	do {							\
> -		spin_unlock(lock);				\
> -		local_irq_restore(flags);			\
> +		spin_unlock_irqrestore(lock, flags);		\
>  		preempt_check_resched();			\
>  	} while (0)

Actually this perhaps hurts lockdep. We want to keep the
arch_spin_(un)lock() versions because each spin_lock() and spin_unlock()
needs to be verified by lockdep. Lockdep also verifies mutex locks. But
with this change, for every mutex, it's going to also analyze a
spin_lock and spin_unlock twice each (one for mutex lock and one for
unlock). As this is just locking the mutex internals, it may not be
necessary to debug it via lockdep. Hence we probably want to keep the
arch_* version.

> 
> But we should hear from the -rt guys (at least!) before proceeding with
> this, please.

-rt fine, lockdep not so fine.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/