Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

[David Howells]

We are using spin_is_locked() in a few places to give a warning or an oops if
either a spinlock is not held or if it is held.  I'm not sure all of these are
safe.

Take uas_try_complete() in drivers/usb/storage/uas.c which does:

	WARN_ON(!spin_is_locked(&devinfo->lock));

or fscache_start_operations() which does:

	ASSERT(spin_is_locked(&object->lock));

These will unconditionally fail under sometimes because under certain
conditions spin_is_locked() is hardwired to 0 (ie. not locked) when actually
we're in a place where the spinlock _should_ be locked, and we should get a
non-zero return.


Would it be reasonable to add a spin_is_not_locked() function for use when we
expect it not to be locked and then use spin_is_locked() only when we expect it
to be locked?

Thanks to Milosz Tanski for spotting this one.

David

Re: Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

[Linus Torvalds]

On Thu, May 23, 2013 at 7:50 AM, David Howells <dhowells@redhat.com> wrote:
>
> We are using spin_is_locked() in a few places to give a warning or an oops if
> either a spinlock is not held or if it is held.  I'm not sure all of these are
> safe.

No, they're not. On SMP, you can get spurious "it's locked" (because
somebody *else* took the lock on another CPU) and on UP you'll always
get "it's unlocked".

So it's never safe to check the state, at least not without checking
for SMP or UP (and realizing that in the SMP case you can only assert
that it's held).

I guess we could change the UP case to always return "it's locked".
But since you'd better know what you're doing with "spin_is_locked()",
I don't think it's worth it making it easier to use.

> Take uas_try_complete() in drivers/usb/storage/uas.c which does:
>
>         WARN_ON(!spin_is_locked(&devinfo->lock));

Pure garbage. That's a debug thing that should not exist.

> or fscache_start_operations() which does:
>
>         ASSERT(spin_is_locked(&object->lock));

Same thing.

We do *not* want to add some crazy "spin_is_nt_locked". We just want
to get rid of these idiotic debug tests.

Note that even on SMP, spin_is_locked() can end up being bad. If this
whole memory transaction thing takes off, testing the lock is possibly
going to abort the transaction.

So I'd suggest removing it entirely. Drivers have absolutely no place
doing crap like this. We could add some particular
"assert_spin_lock_held()" that only ends up existing if spinlock
debugging is enabled or something, and make it clear that it is purely
a debug feature (and it verifies that *this* process holds the lock,
using the debug fields), not a "test if something is locked" or not.

             Linus

Re: Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> We do *not* want to add some crazy "spin_is_nt_locked". We just want
> to get rid of these idiotic debug tests.

Generally, I think you are right, though there are also some checks in
deallocation routines that check that a spinlock is not currently held before
releasing the memory holding it - should those be allowed to stay?  I'd be
tempted to wrap the whole check in something, perhaps an "spin_lock_uninit()"
and move the check to a header file.  Would this be useful for lockdep or
anything like that?

David

Re: Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

[Ryan Mallon]

On 24/05/13 01:12, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> We do *not* want to add some crazy "spin_is_nt_locked". We just want
>> to get rid of these idiotic debug tests.
> 
> Generally, I think you are right, though there are also some checks in
> deallocation routines that check that a spinlock is not currently held before
> releasing the memory holding it - should those be allowed to stay?  I'd be
> tempted to wrap the whole check in something, perhaps an "spin_lock_uninit()"
> and move the check to a header file.  Would this be useful for lockdep or
> anything like that?

lockdep has lockdep_assert_held(), which might be what you want. Though
it looks like it possibly also has the false positive issues on SMP?

~Ryan

Re: Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

[Ryan Mallon]

On 24/05/13 01:12, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> We do *not* want to add some crazy "spin_is_nt_locked". We just want
>> to get rid of these idiotic debug tests.
> 
> Generally, I think you are right, though there are also some checks in
> deallocation routines that check that a spinlock is not currently held before
> releasing the memory holding it - should those be allowed to stay?  I'd be
> tempted to wrap the whole check in something, perhaps an "spin_lock_uninit()"
> and move the check to a header file.  Would this be useful for lockdep or
> anything like that?

lockdep has lockdep_assert_held(), which might be what you want. Though
it looks like it possibly also has the false positive issues on SMP?

~Ryan

Re: Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

[Ingo Molnar]

* Ryan Mallon <rmallon@gmail.com> wrote:

> On 24/05/13 01:12, David Howells wrote:
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> >> We do *not* want to add some crazy "spin_is_nt_locked". We just want
> >> to get rid of these idiotic debug tests.
> > 
> > Generally, I think you are right, though there are also some checks in
> > deallocation routines that check that a spinlock is not currently held before
> > releasing the memory holding it - should those be allowed to stay?  I'd be
> > tempted to wrap the whole check in something, perhaps an "spin_lock_uninit()"
> > and move the check to a header file.  Would this be useful for lockdep or
> > anything like that?
> 
> lockdep has lockdep_assert_held(), which might be what you want. Though 
> it looks like it possibly also has the false positive issues on SMP?

There should be no false positive race in the case that matters: if you 
are expecting to always hold the lock at that point, and want to make sure 
(if lock debugging is enabled), that it's truly held.

Thanks,

	Ingo