[PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Kumar Gala]

For some reason __raw_spin_is_locked() has been returning false for the
uni-processor, non-CONFIG_DEBUG_SPINLOCK.  The UP + CONFIG_DEBUG_SPINLOCK
handles this correctly.

Found this by enabling CONFIG_DEBUG_VM on PPC and hitting always hitting
a BUG_ON that was testing to make sure the pte_lock was held.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

Linus, a fix for 2.6.31

 include/linux/spinlock_up.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/spinlock_up.h b/include/linux/spinlock_up.h
index d4841ed..2b372e0 100644
--- a/include/linux/spinlock_up.h
+++ b/include/linux/spinlock_up.h
@@ -57,7 +57,7 @@ static inline void __raw_spin_unlock(raw_spinlock_t *lock)
 #define __raw_write_unlock(lock)	do { (void)(lock); } while (0)
 
 #else /* DEBUG_SPINLOCK */
-#define __raw_spin_is_locked(lock)	((void)(lock), 0)
+#define __raw_spin_is_locked(lock)	((void)(lock), 1)
 /* for sched.c and kernel_lock.c: */
 # define __raw_spin_lock(lock)		do { (void)(lock); } while (0)
 # define __raw_spin_lock_flags(lock, flags)	do { (void)(lock); } while (0)
-- 
1.6.0.6

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Linus Torvalds]

On Tue, 18 Aug 2009, Kumar Gala wrote:
>
> For some reason __raw_spin_is_locked() has been returning false for the
> uni-processor, non-CONFIG_DEBUG_SPINLOCK.  The UP + CONFIG_DEBUG_SPINLOCK
> handles this correctly.
> 
> Found this by enabling CONFIG_DEBUG_VM on PPC and hitting always hitting
> a BUG_ON that was testing to make sure the pte_lock was held.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> 
> Linus, a fix for 2.6.31

This really isn't all that clear.

The thing is, some people may assert that a lock is held, but others could 
easily be looping until it's not held using something like

	while (spin_is_locked(lock))
		cpu_relax();

so it's hard to tell whether it should return true or false in the case 
where spin-locking simply doesn't exist.

		Linus

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Steven Rostedt]

On Tue, 18 Aug 2009, Linus Torvalds wrote:

> 
> 
> On Tue, 18 Aug 2009, Kumar Gala wrote:
> >
> > For some reason __raw_spin_is_locked() has been returning false for the
> > uni-processor, non-CONFIG_DEBUG_SPINLOCK.  The UP + CONFIG_DEBUG_SPINLOCK
> > handles this correctly.
> > 
> > Found this by enabling CONFIG_DEBUG_VM on PPC and hitting always hitting
> > a BUG_ON that was testing to make sure the pte_lock was held.
> > 
> > Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> > ---
> > 
> > Linus, a fix for 2.6.31
> 
> This really isn't all that clear.
> 
> The thing is, some people may assert that a lock is held, but others could 
> easily be looping until it's not held using something like
> 
> 	while (spin_is_locked(lock))
> 		cpu_relax();

Wouldn't something like that be really racey? And anyone doing such a 
thing had better have that code within an #ifdef CONFIG_SMP.

> 
> so it's hard to tell whether it should return true or false in the case 
> where spin-locking simply doesn't exist.

Actually, I did have a case where I would use it and would expect a return 
of 0. That was in the experimental printk code to see if it was safe to 
wakeup the klogd. I once had a check of the current cpu runqueue lock is 
locked, and if it was, not to wake up klogd. I'm sure there's other cases 
like this as well.

Thinking about it, UP probably should have spin_is_locked always return 
false, but if you want to make sure you are not in a critical section 
with the lock not held, then use assert_spin_locked, which on UP should be 
a nop.

-- Steve

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Linus Torvalds]

On Tue, 18 Aug 2009, Steven Rostedt wrote:
>
> > The thing is, some people may assert that a lock is held, but others could 
> > easily be looping until it's not held using something like
> > 
> > 	while (spin_is_locked(lock))
> > 		cpu_relax();
> 
> Wouldn't something like that be really racey? And anyone doing such a 
> thing had better have that code within an #ifdef CONFIG_SMP.

Sure, it's hopefully inside a #ifdef CONFIG_SMP.

And no, it's not necessarily racy. Sure, it's race in itself if that's all 
you are doing, but I could imagine writing that kind of code if I knew 
some lock was likely held, and I wanted to avoid doing a "try_lock()" 
until it got released.

The point is, "spin_is_locked()" is simply not a well-defined operation in 
this case. It could go either way.

And for the original case, we actually have a function for that:

	assert_spin_locked(x)

which goes away on UP. Exactly because

	BUG_ON(!spin_is_locked(x))

is not a good thing to do!

> > so it's hard to tell whether it should return true or false in the case 
> > where spin-locking simply doesn't exist.
> 
> Actually, I did have a case where I would use it and would expect a return 
> of 0. That was in the experimental printk code to see if it was safe to 
> wakeup the klogd. I once had a check of the current cpu runqueue lock is 
> locked, and if it was, not to wake up klogd. I'm sure there's other cases 
> like this as well.

Yeah, "spin_is_locked()" can be useful for those kinds of things. A 
heuristic for whether we should do something based on whether some other 
CPU holds it (or we migth have recursion).

Exactly like it can be useful for doing the BUG_ON thing. But in both 
cases it's a bit iffy.

> Thinking about it, UP probably should have spin_is_locked always return 
> false, but if you want to make sure you are not in a critical section 
> with the lock not held, then use assert_spin_locked, which on UP should be 
> a nop.

That's what we do. That said, I also think we should generally try to 
avoid the kind of code that depends on spin_is_locked always returning 
false, for the same reason we should try to avoid any code that depends on 
it always returning true. 

		Linus

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Steven Rostedt]

On Tue, 18 Aug 2009, Linus Torvalds wrote:
> 
> > Thinking about it, UP probably should have spin_is_locked always return 
> > false, but if you want to make sure you are not in a critical section 
> > with the lock not held, then use assert_spin_locked, which on UP should be 
> > a nop.
> 
> That's what we do. That said, I also think we should generally try to 
> avoid the kind of code that depends on spin_is_locked always returning 
> false, for the same reason we should try to avoid any code that depends on 
> it always returning true. 

Perhaps we can deprecate spin_is_locked and replace it with 
"expect_spin_locked" and "expect_spin_unlocked" which on SMP would return 
true and false respectively if the lock was locked. But both would always 
return true on UP.

Or do some thing similar, that would remove the ambiguity on UP.

-- Steve

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Kumar Gala]

On Aug 18, 2009, at 7:07 PM, Steven Rostedt wrote:

> On Tue, 18 Aug 2009, Linus Torvalds wrote:
>>
>>> Thinking about it, UP probably should have spin_is_locked always  
>>> return
>>> false, but if you want to make sure you are not in a critical  
>>> section
>>> with the lock not held, then use assert_spin_locked, which on UP  
>>> should be
>>> a nop.
>>
>> That's what we do. That said, I also think we should generally try to
>> avoid the kind of code that depends on spin_is_locked always  
>> returning
>> false, for the same reason we should try to avoid any code that  
>> depends on
>> it always returning true.
>
> Perhaps we can deprecate spin_is_locked and replace it with
> "expect_spin_locked" and "expect_spin_unlocked" which on SMP would  
> return
> true and false respectively if the lock was locked. But both would  
> always
> return true on UP.
>
> Or do some thing similar, that would remove the ambiguity on UP.

I agree its a little too easy to abuse spin_is_locked.  However we  
should be consistent between spin_is_locked on UP between with and  
without CONFIG_DEBUG_SPINLOCK enabled.  How much of this do we want to  
try and address in .31?

The PPC test really should be using assert_spin_locked and I'll send a  
patch to Ben for that.

- k

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Linus Torvalds]

On Tue, 18 Aug 2009, Kumar Gala wrote:
> 
> I agree its a little too easy to abuse spin_is_locked.  However we should be
> consistent between spin_is_locked on UP between with and without
> CONFIG_DEBUG_SPINLOCK enabled.

No we shouldn't.

With CONFIG_DEBUG_SPINLOCK, you have an actual lock variable for debugging 
purposes, so spin_is_locked() can clearly return a _valid_ answer, and 
should do so.

Without DEBUG_SPINLOCK, there isn't any answer to return.

So there's no way we can or should be consistent. In one case an answer 
exists, in another one the answer is meaningless and doesn't exist.

> How much of this do we want to try and address in .31?

Absolutely nothing.

> The PPC test really should be using assert_spin_locked and I'll send a patch
> to Ben for that.

Yes, that's the correct fix.

		Linus

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Olivier Galibert]

On Tue, Aug 18, 2009 at 07:40:16PM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 18 Aug 2009, Kumar Gala wrote:
> > 
> > I agree its a little too easy to abuse spin_is_locked.  However we should be
> > consistent between spin_is_locked on UP between with and without
> > CONFIG_DEBUG_SPINLOCK enabled.
> 
> No we shouldn't.
> 
> With CONFIG_DEBUG_SPINLOCK, you have an actual lock variable for debugging 
> purposes, so spin_is_locked() can clearly return a _valid_ answer, and 
> should do so.
> 
> Without DEBUG_SPINLOCK, there isn't any answer to return.
> 
> So there's no way we can or should be consistent. In one case an answer 
> exists, in another one the answer is meaningless and doesn't exist.

I always thought behaviour should be consistent between code with
debugging on and code without.  Otherwise you may end up with cases of
"it starts working when I turn on debugging" which are a pain to fix.
Has something changed?

Or in other words, do you think lockdep should try solving deadlocks
instead of just reporting them for instance?

  OG.

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Peter Zijlstra]

On Wed, 2009-08-19 at 11:31 +0200, Olivier Galibert wrote:
> On Tue, Aug 18, 2009 at 07:40:16PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Tue, 18 Aug 2009, Kumar Gala wrote:
> > > 
> > > I agree its a little too easy to abuse spin_is_locked.  However we should be
> > > consistent between spin_is_locked on UP between with and without
> > > CONFIG_DEBUG_SPINLOCK enabled.
> > 
> > No we shouldn't.
> > 
> > With CONFIG_DEBUG_SPINLOCK, you have an actual lock variable for debugging 
> > purposes, so spin_is_locked() can clearly return a _valid_ answer, and 
> > should do so.
> > 
> > Without DEBUG_SPINLOCK, there isn't any answer to return.
> > 
> > So there's no way we can or should be consistent. In one case an answer 
> > exists, in another one the answer is meaningless and doesn't exist.
> 
> I always thought behaviour should be consistent between code with
> debugging on and code without.  Otherwise you may end up with cases of
> "it starts working when I turn on debugging" which are a pain to fix.
> Has something changed?
> 
> Or in other words, do you think lockdep should try solving deadlocks
> instead of just reporting them for instance?

The point is spin_is_locked() is a broken interface in that respect. Its
plain impossible to give the right answer.


Suppose there's code doing:

  /*
   * Ensure we don't have foo lock taken, because that would cause
   * lock inversion under bar lock.
   */
  BUG_ON(spin_is_locked(&foo));
  spin_lock(&bar);

and other code doing:

  /*
   * Ensure we've got foo locked because it protects bar
   */
  BUG_ON(!spin_is_locked(&foo));
  bar = fancy;

What value should you return when locks don't exist (which is the case
for UP)?


There simply is no right answer other than: don't use spin_is_locked().

Re: [PATCH] spinlock: __raw_spin_is_locked() should return true for UP

[Scott Wood]

On Tue, Aug 18, 2009 at 04:52:20PM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 18 Aug 2009, Steven Rostedt wrote:
> >
> > > The thing is, some people may assert that a lock is held, but others could 
> > > easily be looping until it's not held using something like
> > > 
> > > 	while (spin_is_locked(lock))
> > > 		cpu_relax();
> > 
> > Wouldn't something like that be really racey? And anyone doing such a 
> > thing had better have that code within an #ifdef CONFIG_SMP.
> 
> Sure, it's hopefully inside a #ifdef CONFIG_SMP.
> 
> And no, it's not necessarily racy. Sure, it's race in itself if that's all 
> you are doing, but I could imagine writing that kind of code if I knew 
> some lock was likely held, and I wanted to avoid doing a "try_lock()" 
> until it got released.

So you'd basically have the effect of a spin_lock(), except with the
bonus of breaking RT hacks that do something other than spin, and
preventing arch code from doing certain types of relaxation that are only
appropriate when waiting on a lock (such as mdors on powerpc, which
de-emphasizes until a reservation is broken).

Not exactly something we should encourage, IMHO.

-Scott