ULE: set td_oncpu before switching.

[markj]

Summary

This ensures that updates to td_oncpu are synchronized by the thread
lock, as they were before r355784, and like td_state.

Test Plan

I was running procstat -kka in a loop concurrent with a buildkernel loop. Without the patch, stack_save_td_running() generates an unclaimed NMI quite quickly. I will ask Peter to test as well.

[kib]

In fact, there is at least one place that is affected, I remembered.

Please look at x86/mp_x86.c:smp_after_idle_runnable(). It might cause the code to detect that the initial boot stack for AP is unused while cpu_throw() still not switched to the stack of the idle thread.

[markj]

In D23270#510267, @kib wrote:

In fact, there is at least one place that is affected, I remembered.

Please look at x86/mp_x86.c:smp_after_idle_runnable(). It might cause the code to detect that the initial boot stack for AP is unused while cpu_throw() still not switched to the stack of the idle thread.

Instead of depending on scheduler internals, what do you think of instead calling smp_rendezvous()? Once it returns we know that all APs have enabled interrupts, and they can only do that after cpu_throw() switches to the idle thread and fork_exit() reenables interrupts.

[kib]

In D23270#510275, @markj wrote:

Instead of depending on scheduler internals, what do you think of instead calling smp_rendezvous()? Once it returns we know that all APs have enabled interrupts, and they can only do that after cpu_throw() switches to the idle thread and fork_exit() reenables interrupts.

I propose D23276. I do not like so early use of rendezvous, esp. in the situation where the target CPU might be not yet fully initialized. The wait for curthread does not depend on the scheduler at all.

[markj]

In sched_switch(), update td_oncpu before releasing the thread queue
lock.

[jeff]

sys/kern/sched_ule.c

2147 ↗

(On Diff #67104)

Do we want to also clear this under the scheduler lock?

[markj]

sys/kern/sched_ule.c

2147 ↗

(On Diff #67104)

At this point td is still protected by blocked_lock, so I wouldn't think it's necessary. The locking annotation for td_oncpu just says that it is protect by the thread lock.

[markj]

I reverted this change. There is a race triggered by sched_bind():

CPU 1:

td (curthread) binds to CPU 2
- In sched_switch(), unlock CPU 1's thread queue, move td to
  CPU 2's runqueue, relock CPU 1's thread queue to choose a new
  thread.  td's lock is the blocked lock and will be changed to CPU's
  tdq lock after the switch.

CPU 2:

idle thread sees load on the CPU's run queue
- Call sched_switch(), lock CPU 2's runqueue, remove td and prepare to
  switch to it.  Set td->td_oncpu = 2 and drop the thread queue lock.

CPU 1:

set td->td_oncpu = NOCPU and call cpu_switch()
- cpu_switch() won't actually switch to td until it is unblocked by
  CPU 1.

So now that we set td_oncpu before switching, we are left running with
td_oncpu == NOCPU. (I reverted the change this morning.) I guess this
means that td_oncpu has to be set only after returning from
cpu_switch(). In particular, td_oncpu is no longer protected by the
thread lock. I will fix some code in stack_machdep.c that was found to
depend on this synchronization, and will spend time auditing the rest
of the td_oncpu accesses.