improve spinlock debugging

Keep track of how many locks are being held at once, and check if it is zero at switch_to() time. You can also do this to measure things like max number of locks held at once and other statistics.

I added the first bit to sparc64 while hunting down a bug.

CONFIG_DEBUG_SPINLOCK only adds spinlock tests for SMP builds. The attached patch adds runtime checks for uniprocessor builds.

Tested on i386/UP, but it should work on all platforms. It contains runtime checks for:

- missing initialization
- recursive lock
- double unlock
- incorrect use of spin_is_locked() or spin_trylock() [both function do not work as expected on uniprocessor builds]
The next step are checks for spinlock ordering mismatches.

Which other runtime checks are possible?
Tests for correct _irq usage are not possible, several drivers use disable_irq().

--
Manfred

spin_lockirq

spin_unlock

restore_irq

Quoted 5 lines · george anzinger

spin_lockirq

spin_unlock

restore_irq

Given this order, couldn't we _always_ not touch the preempt count since irq's are off?

Further, since I doubt we ever see:

spin_lock_irq
restore_irq
spin_unlock

and the common use is:

spin_lock_irq
spin_unlock_irq

Isn't it safe to have spin_lock_irq *never* touch the preempt count?

Quoted 5 lines

spin_lockirq

spin_unlock

restore_irq

Quoted 2 lines

Given this order, couldn't we _always_ not touch the preempt count since irq's are off?

Not with the current spinlock usage in the kernel. spin_lock/spin_unlock are used both nested when interrupts are known to be disabled (as above) or, more commonly,

spin_lock_irqsave(a, flags)

spin_lock(b)

spin_unlock(b)

spin_unlock_irqrestore(a, flags)

and when interrupts are enabled:

spin_lock(c)

spin_unlock(c)

We don't need to preempt count the former but we do the latter, but there's no way to tell the difference without a runtime check for interrupt state.

In IRIX we changed the name of the former, nested versions to:

spin_lock_irqsave(a, flags)

nested_spin_lock(b)

nested_spin_unlock(b)

spin_unlock_irqrestore(a, flags)

The nested version contained an assertion that interrupts were disabled in DEBUG kernels. We wouldn't need to count the nested_spin_lock versions.

Quoted 5 lines

Further, since I doubt we ever see:

spin_lock_irq
restore_irq
spin_unlock

I hope not, since that would be a bug.

Quoted 6 lines

and the common use is:

spin_lock_irq
spin_unlock_irq

Isn't it safe to have spin_lock_irq *never* touch the preempt count?

No, because of

Quoted 2 lines

Quoted 2 lines

spin_lockirq

Quoted 3 lines

Quoted 3 lines

spin_unlock

restore_irq

(which does occasionally occur in the kernel). The spin_unlock is going to decrement the count, so the spin_lock_irqsave must increment it. If we had, and used, a nested_spin_unlock, we could then have spin_lock_irq never touch the preempt count.

[And if we could guarantee that all spinlocks we held for only a few 10s of microseconds at the most (a big "if"), we could make them all into spin_lock_irqs and then we wouldn't need the preempt count at all. This is how REAL/IX and IRIX implemented kernel preemption.]

Right, I meant just the spin_lock_irq case, which would be fine except for the case where:

spin_lock_irq
spin_unlock
restore_irq

to solve this, we need a spin_unlock_irq_on macro that didn't touch the preemption count.

Hi,

Quoted 7 lines · Robert Love

Right, I meant just the spin_lock_irq case, which would be fine except for the case where:

spin_lock_irq
spin_unlock
restore_irq

Quoted 2 lines

to solve this, we need a spin_unlock_irq_on macro that didn't touch the preemption count.

Has someone a real example of something like this?
I'd suspect someone is trying a (questionable) micro optimization or is holding the lock for too long anyway. Instead of adding more macros, maybe it's better to look closely whether something needs fixing.

For example, preemption currently counts
up on spin_lock and disable irq, counting the spin_lockirq twice. In

(Oh, and since the irq
inhibits preemption all by itself, we don't need to count it either.)

Quoted 2 lines · george anzinger

For example, preemption currently counts
up on spin_lock and disable irq, counting the spin_lockirq twice. In

That's not correct: we don't count it twice because we don't count local_irq_disable etc., only the spin locks. Because...

Quoted 2 lines

(Oh, and since the irq
inhibits preemption all by itself, we don't need to count it either.)

...so we don't.

Quoted 5 lines · george anzinger

spin_lockirq

spin_unlock

restore_irq

Given this order, couldn't we _always_ not touch the preempt count since irq's are off?

Further, since I doubt we ever see:

spin_lock_irq
restore_irq
spin_unlock

and the common use is:

spin_lock_irq
spin_unlock_irq

Isn't it safe to have spin_lock_irq *never* touch the preempt count?

NO. The problem is the first example above. The spin_unlock will down count, but the spin_lockirq did NOT do the paired up count (been there, done that). This is where we need the spin_unlock_no_irq_restore.

Quoted 7 lines · Manfred Spraul

CONFIG_DEBUG_SPINLOCK only adds spinlock tests for SMP builds. The attached patch adds runtime checks for uniprocessor builds.

Tested on i386/UP, but it should work on all platforms. It contains runtime checks for:

Quoted 6 lines

- missing initialization
- recursive lock
- double unlock
- incorrect use of spin_is_locked() or spin_trylock() [both function do not work as expected on uniprocessor builds]
The next step are checks for spinlock ordering mismatches.

What do you consider an order mismatch? I would like to see this:

Run time checks for xxx_irq when irq is already off seem reasonable. The implication is that the xxx_unlockirq will then turn it on, which most likely is an error. Also, see above about rolling assumptions in to the macro name.

Quoted 9 lines · george anzinger

Quoted 7 lines · Manfred Spraul

CONFIG_DEBUG_SPINLOCK only adds spinlock tests for SMP builds. The attached patch adds runtime checks for uniprocessor builds.

Tested on i386/UP, but it should work on all platforms. It contains runtime checks for:

Quoted 9 lines

Quoted 6 lines

- missing initialization
- recursive lock
- double unlock
- incorrect use of spin_is_locked() or spin_trylock() [both function do not work as expected on uniprocessor builds]
The next step are checks for spinlock ordering mismatches.

What do you consider an order mismatch? I would like to see this:

        spin_lock(a);
        spin_lock(b);

and somewhere else

        spin_lock(b);
        spin_lock(a);

Quoted 6 lines

Run time checks for xxx_irq when irq is already off seem reasonable. The implication is that the xxx_unlockirq will then turn it on, which most likely is an error. Also, see above about rolling assumptions in to the macro name.

Unfortuantely there are still a few special cases where spin_lock_irq() with already disabled interrupts is both intentional and correct^wnot buggy (search for sleep_on and cli() through the lkml archives).

And there are optimizations such as

        spin_lock_bh(a);
        spin_unlock(b);
        spin_lock(b);
        spin_unlock_bh(b);

Tested on i386/UP, but it should work on all platforms. It contains runtime checks for:

- missing initialization
- recursive lock
- double unlock
- incorrect use of spin_is_locked() or spin_trylock() [both function do not work as expected on uniprocessor builds]
The next step are checks for spinlock ordering mismatches.

Which other runtime checks are possible?

It's very useful to log when a lock(irq) is held more than xx ms and who is the caller. Is it possible ?

What do you think of the idea below? We create arch specific spinlock functions as __foo and wrap the debugging bits around them as foo. This should also allow us to unify the UP and SMP spinlock debugging somewhat.