svn commit: r325734 - head/sys/amd64/amd64

[Bruce Evans]

On Sun, 12 Nov 2017, Bruce Evans wrote:

> On Sun, 12 Nov 2017, Mateusz Guzik wrote:
>
>> Log:
>>  amd64: stop nesting preemption counter in spinlock_enter
>> 
>>  Discussed with:	jhb
>
> This seems to break it.  i386 still has the old code.
> ...
> The main broken case is:
> - both levels initially 0
> - disable interrupts
> - raise spinlock count to 1
> - bad race window until critical_enter() is called.  Disabling hardware
>  interrupts doesn't prevent exceptions like debugger traps or NMIs.
>  Debuuger trap handlers shouldn't use critical sections or (spin)
>  mutexes, but NMI handlers might.  When an exception handler calls
>  spinlock_enter(), this no longer gives a critical section and bad
>  things like context switches occur if the handler calls critical_enter/
>  exit().
> ...
> I don't like this change.  The nested counting is easier to understand,
> and the nested case is very rare and the critical section part of it is
> very efficient (then critical_exit() is just lowering the level).  Old
> ...
> I think the nested case is only for recursive spinlocks.  So NMI handlers
> should not use any spinlocks and the new bug is small (NMI handlers should
> not use non-recursive spinlocks since they would deadlock, and should not
> use recursive spinlocks since they don't work).  NMI handlers aren't that
> careful.  They call printf(), and even the message buffer has been broken
> to use non-recursive spinlocks.

Actually, it is valid for NMI handlers to use spinlocks via mtx_trylock_spin()
in the non-kdb non-panic case, and that is what my fixes for printf() do.

I had confused "nesting preemption counter" (td_critnest) with interrupt
nesting (the bogus td_intr_nesting_level).  td_critnest was incremented
for every concurrently held spinlock, so it could grow quite large without
any interrupt/exception recursion.  So the micro-optimization of td_critnest
is useful if it is faster and fixed to work.

Bruce