[PATCH 1/2] lockstat: fix a typo

[PATCH 1/2] lockstat: fix a typo

[Yuanhan Liu]

It should be CONFIG_LOCK_STAT

Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 Documentation/lockstat.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/lockstat.txt b/Documentation/lockstat.txt
index cef00d4..dd2f7b2 100644
--- a/Documentation/lockstat.txt
+++ b/Documentation/lockstat.txt
@@ -65,7 +65,7 @@ that had to wait on lock acquisition.
 
  - CONFIGURATION
 
-Lock statistics are enabled via CONFIG_LOCK_STATS.
+Lock statistics are enabled via CONFIG_LOCK_STAT.
 
  - USAGE
 
-- 
1.7.7.6

[PATCH 2/2] mutex: use spin_[un]lock instead of arch_spin_[un]lock

[Yuanhan Liu]

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.

Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 kernel/mutex-debug.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/mutex-debug.h b/kernel/mutex-debug.h
index 0799fd3..556c0bc 100644
--- 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)
-- 
1.7.7.6

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

[Ingo Molnar]

* Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.
> 
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  kernel/mutex-debug.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/mutex-debug.h b/kernel/mutex-debug.h
> index 0799fd3..556c0bc 100644
> --- 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);			\

But in that case it could probably use the spin_lock_irqsave() 
primitive, right?

>  		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();			\

And here spin_unlock_irqrestore().

Thanks,

	Ingo

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

[Yuanhan Liu]

On Thu, Jan 24, 2013 at 10:58:07AM +0100, Ingo Molnar wrote:
> 
> * Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.
> > 
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> >  kernel/mutex-debug.h |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/mutex-debug.h b/kernel/mutex-debug.h
> > index 0799fd3..556c0bc 100644
> > --- 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);			\
> 
> But in that case it could probably use the spin_lock_irqsave() 
> primitive, right?

Right, in that case I should use spin_lock_irqsave.

But one question, why we use spin_lock at kernel/mutex.h,
while use 'local_irq_save(); arch_spin_lock' at kernel/mutex-debug.h?

Shouldn't we keep it consistent? Say use spin_lock_irqsave?

Thanks.

	--yliu
> 
> >  		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();			\
> 
> And here spin_unlock_irqrestore().
> 
> Thanks,
> 
> 	Ingo

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

[Ingo Molnar]

* Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> On Thu, Jan 24, 2013 at 10:58:07AM +0100, Ingo Molnar wrote:
> > 
> > * Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.
> > > 
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > ---
> > >  kernel/mutex-debug.h |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/mutex-debug.h b/kernel/mutex-debug.h
> > > index 0799fd3..556c0bc 100644
> > > --- 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);			\
> > 
> > But in that case it could probably use the spin_lock_irqsave() 
> > primitive, right?
> 
> Right, in that case I should use spin_lock_irqsave.
> 
> But one question, why we use spin_lock at kernel/mutex.h, 
> while use 'local_irq_save(); arch_spin_lock' at 
> kernel/mutex-debug.h?
> 
> Shouldn't we keep it consistent? Say use spin_lock_irqsave?

I think we did it to increase performance with lockdep enabled - 
this particular lockdep annotation, given the short codepaths, 
is not that hard to verify - and if it breaks it will break a 
thousand mutex locking places in the kernel.

So maybe it's better to leave it alone - maybe add a comment 
that explains the reason.

Thanks,

	Ingo

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

[Yuanhan Liu]

On Thu, Jan 24, 2013 at 11:14:50AM +0100, Ingo Molnar wrote:
> 
> * Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> 
> > On Thu, Jan 24, 2013 at 10:58:07AM +0100, Ingo Molnar wrote:
> > > 
> > > * Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.
> > > > 
> > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > > ---
> > > >  kernel/mutex-debug.h |    4 ++--
> > > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/kernel/mutex-debug.h b/kernel/mutex-debug.h
> > > > index 0799fd3..556c0bc 100644
> > > > --- 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);			\
> > > 
> > > But in that case it could probably use the spin_lock_irqsave() 
> > > primitive, right?
> > 
> > Right, in that case I should use spin_lock_irqsave.
> > 
> > But one question, why we use spin_lock at kernel/mutex.h, 
> > while use 'local_irq_save(); arch_spin_lock' at 
> > kernel/mutex-debug.h?
> > 
> > Shouldn't we keep it consistent? Say use spin_lock_irqsave?
> 
> I think we did it to increase performance with lockdep enabled - 
> this particular lockdep annotation, given the short codepaths, 
> is not that hard to verify - and if it breaks it will break a 
> thousand mutex locking places in the kernel.

Thanks for the explanation.
> 
> So maybe it's better to leave it alone - maybe add a comment 
> that explains the reason.

Sorry, I may not get your point clearly. Should I make another patch to
convert 'local_irq_save(..); arch_spin_lock(..);' at kernel/mutex-debug.h
to spin_lock_irqsave() then?

Thanks.

	--yliu

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

[Ingo Molnar]

* Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> On Thu, Jan 24, 2013 at 11:14:50AM +0100, Ingo Molnar wrote:
> > 
> > * Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> > 
> > > On Thu, Jan 24, 2013 at 10:58:07AM +0100, Ingo Molnar wrote:
> > > > 
> > > > * Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.
> > > > > 
> > > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > > > ---
> > > > >  kernel/mutex-debug.h |    4 ++--
> > > > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/mutex-debug.h b/kernel/mutex-debug.h
> > > > > index 0799fd3..556c0bc 100644
> > > > > --- 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);			\
> > > > 
> > > > But in that case it could probably use the spin_lock_irqsave() 
> > > > primitive, right?
> > > 
> > > Right, in that case I should use spin_lock_irqsave.
> > > 
> > > But one question, why we use spin_lock at kernel/mutex.h, 
> > > while use 'local_irq_save(); arch_spin_lock' at 
> > > kernel/mutex-debug.h?
> > > 
> > > Shouldn't we keep it consistent? Say use spin_lock_irqsave?
> > 
> > I think we did it to increase performance with lockdep enabled - 
> > this particular lockdep annotation, given the short codepaths, 
> > is not that hard to verify - and if it breaks it will break a 
> > thousand mutex locking places in the kernel.
> 
> Thanks for the explanation.
> > 
> > So maybe it's better to leave it alone - maybe add a comment 
> > that explains the reason.
> 
> Sorry, I may not get your point clearly. Should I make another 
> patch to convert 'local_irq_save(..); arch_spin_lock(..);' at 
> kernel/mutex-debug.h to spin_lock_irqsave() then?

No, I'd suggest to add a comment that explains why there's no 
lockdep annotation in that place.

Thanks,

	Ingo

[tip:core/locking] locking/stat: Fix a typo

[tip-bot for Yuanhan Liu]

Commit-ID:  5b43715b9d27e4e6620264113169bb8e4e607205
Gitweb:     http://git.kernel.org/tip/5b43715b9d27e4e6620264113169bb8e4e607205
Author:     Yuanhan Liu <yuanhan.liu@linux.intel.com>
AuthorDate: Thu, 24 Jan 2013 17:22:44 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 24 Jan 2013 10:59:43 +0100

locking/stat: Fix a typo

s/STATS/STAT

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1359019365-23646-1-git-send-email-yuanhan.liu@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/lockstat.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/lockstat.txt b/Documentation/lockstat.txt
index cef00d4..dd2f7b2 100644
--- a/Documentation/lockstat.txt
+++ b/Documentation/lockstat.txt
@@ -65,7 +65,7 @@ that had to wait on lock acquisition.
 
  - CONFIGURATION
 
-Lock statistics are enabled via CONFIG_LOCK_STATS.
+Lock statistics are enabled via CONFIG_LOCK_STAT.
 
  - USAGE
 

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

[Andrew Morton]

On Thu, 24 Jan 2013 17:22:45 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.


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)

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

Re: [PATCH 2/2] mutex: use spin_[un]lock instead of arch_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@linux.intel.com> 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

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

[Andrew Morton]

On Thu, 24 Jan 2013 19:50:57 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

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

Thanks (tglx) for writing a useful changelog ;)

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

In what way is this actually a problem?  lockdep will have more work to
do (and given the frequency of mutex_lock/unlock, that overhead may be
significant).  Anything else?

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

[Steven Rostedt]

On Thu, 2013-01-24 at 16:56 -0800, Andrew Morton wrote:
>  
> > 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.
> 
> In what way is this actually a problem?  lockdep will have more work to
> do (and given the frequency of mutex_lock/unlock, that overhead may be
> significant).  Anything else?

Ingo probably can answer this better than I can. I'll let him do it when
he wakes up tomorrow.

-- Steve

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

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 24 Jan 2013 17:22:45 +0800
> Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.

So, as per the discussion we don't want this patch, because we 
are using raw locks there to keep mutex lockdep overhead low. 
The value of lockdep-checking such a basic locking primitive is 
minimal - it's rarely tweaked and if it breaks we won't have a 
bootable kernel to begin with.

So instead I suggested a different patch: adding a comment to 
explain why we don't lockdep-cover the mutex code spinlocks.

> 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);		\

Yes, I mentioned that yesterday, but we really don't want the 
change to begin with.

Thanks,

	Ingo

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

[Ingo Molnar]

* nan chen <nachenn@gmail.com> wrote:

> 2013/1/25 Ingo Molnar <mingo@kernel.org>
> 
> >
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > > On Thu, 24 Jan 2013 17:22:45 +0800
> > > Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.
> >
> > So, as per the discussion we don't want this patch, because we
> > are using raw locks there to keep mutex lockdep overhead low.
> > The value of lockdep-checking such a basic locking primitive is
> > minimal - it's rarely tweaked and if it breaks we won't have a
> > bootable kernel to begin with.
> >
> > So instead I suggested a different patch: adding a comment to
> > explain why we don't lockdep-cover the mutex code spinlocks.
> >
> > > 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);         \
> >
> > Yes, I mentioned that yesterday, but we really don't want the
> > change to begin with.
> >
> > Thanks,
> >
> >         Ingo
> >
> 
> Hi,
> 
> Looks like in mutex.h, it does not disable local interrupt.  
> But why the code disable local interrupt in mutex-debug.h?

To protect against preemption I suspect. preempt_disable() could 
be used in the mutex-debug.h variant I suppose.

Thanks,

	Ingo

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

[Ingo Molnar]

* nan chen <nachenn@gmail.com> wrote:

> 2013/1/25 Ingo Molnar <mingo@kernel.org>
> 
> >
> > * nan chen <nachenn@gmail.com> wrote:
> >
> > > 2013/1/25 Ingo Molnar <mingo@kernel.org>
> > >
> > > >
> > > > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > > >
> > > > > On Thu, 24 Jan 2013 17:22:45 +0800
> > > > > Yuanhan Liu <yuanhan.liu@linux.intel.com> 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.
> > > >
> > > > So, as per the discussion we don't want this patch, because we
> > > > are using raw locks there to keep mutex lockdep overhead low.
> > > > The value of lockdep-checking such a basic locking primitive is
> > > > minimal - it's rarely tweaked and if it breaks we won't have a
> > > > bootable kernel to begin with.
> > > >
> > > > So instead I suggested a different patch: adding a comment to
> > > > explain why we don't lockdep-cover the mutex code spinlocks.
> > > >
> > > > > 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);         \
> > > >
> > > > Yes, I mentioned that yesterday, but we really don't want the
> > > > change to begin with.
> > > >
> > > > Thanks,
> > > >
> > > >         Ingo
> > > >
> > >
> > > Hi,
> > >
> > > Looks like in mutex.h, it does not disable local interrupt.
> > > But why the code disable local interrupt in mutex-debug.h?
> >
> > To protect against preemption I suspect. preempt_disable() could
> > be used in the mutex-debug.h variant I suppose.
> >
> > Thanks,
> >
> >         Ingo
> >
> 
> 
> spin_lock() itself already protects against preemption.

Yes, but mutex-debug.h does not use spin_lock(), it uses 
arch_spin_lock():

#define spin_lock_mutex(lock, flags)                    \
        do {                                            \
                struct mutex *l = container_of(lock, struct mutex, wait_lock); \
                                                        \
                DEBUG_LOCKS_WARN_ON(in_interrupt());    \
                local_irq_save(flags);                  \
                arch_spin_lock(&(lock)->rlock.raw_lock);\
                DEBUG_LOCKS_WARN_ON(l->magic != l);     \
        } while (0)

The original question was why mutex-debug.h (unmodified) uses 
irq disabling.

Thanks,

	Ingo

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

[Ingo Molnar]

* nan chen <nachenn@gmail.com> wrote:

> > #define spin_lock_mutex(lock, flags)                    \
> >         do {                                            \
> >                 struct mutex *l = container_of(lock, struct mutex,
> > wait_lock); \
> >                                                         \
> >                 DEBUG_LOCKS_WARN_ON(in_interrupt());    \
> >                 local_irq_save(flags);                  \
> >                 arch_spin_lock(&(lock)->rlock.raw_lock);\
> >                 DEBUG_LOCKS_WARN_ON(l->magic != l);     \
> >         } while (0)
> >
> > The original question was why mutex-debug.h (unmodified) uses
> > irq disabling.
> >
> > Thanks,
> >
> >         Ingo
> >
> 
> So, the reason why you use WARN_ON here is try to keep things 
> going?

What would be the alternative?

Thanks,

	Ingo

[tip:core/locking] locking/stat: Fix a typo

[tip-bot for Yuanhan Liu]

Commit-ID:  0be5c8ff58cf7a66019af2f1236daff731ed318c
Gitweb:     http://git.kernel.org/tip/0be5c8ff58cf7a66019af2f1236daff731ed318c
Author:     Yuanhan Liu <yuanhan.liu@linux.intel.com>
AuthorDate: Thu, 24 Jan 2013 17:22:44 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 19 Feb 2013 08:42:37 +0100

locking/stat: Fix a typo

s/STATS/STAT

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1359019365-23646-1-git-send-email-yuanhan.liu@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/lockstat.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/lockstat.txt b/Documentation/lockstat.txt
index cef00d4..dd2f7b2 100644
--- a/Documentation/lockstat.txt
+++ b/Documentation/lockstat.txt
@@ -65,7 +65,7 @@ that had to wait on lock acquisition.
 
  - CONFIGURATION
 
-Lock statistics are enabled via CONFIG_LOCK_STATS.
+Lock statistics are enabled via CONFIG_LOCK_STAT.
 
  - USAGE