I'm sorry, but I still don't understand the problem. Which scheduler data structures are not initialized at SI_SUB_SMP? Note that "at init_secondary() time" is ambiguous: APs are started fairly early during boot, but they won't call cpu_initclocks_ap() until aps_ready is set, which happens at SI_SUB_SMP, i.e., quite late, and a point where it's perfectly safe to schedule other threads. But we shouldn't be switching to other threads unless the idlethread's spinlock count drops to 0, which shouldn't happen until we reach fork_exit().

In D32797#740166, @markj wrote:

I'm sorry, but I still don't understand the problem. Which scheduler data structures are not initialized at SI_SUB_SMP? Note that "at init_secondary() time" is ambiguous: APs are started fairly early during boot, but they won't call cpu_initclocks_ap() until aps_ready is set, which happens at SI_SUB_SMP, i.e., quite late, and a point where it's perfectly safe to schedule other threads. But we shouldn't be doing that unless the idlethread's spinlock count drops to 0, which shouldn't happen until we reach fork_exit().

Sorry, the description ended up being really bad/inaccurate. I'll rewrite it, but here's the breakdown:

• init_secondary() -> cpu_initclocks_ap() -> handleevents() -> callout_process() -> swi_sched() -> intr_event_schedule_thread() () -> sched_add() -> ... -> sched_setpreempt() -> asserts on curthread->td_lock *before* init_secondary has called sched_throw(NULL)
• curthread == idlethread at that time
• idlethread->td_lock isn't setup until later, at https://cgit.freebsd.org/src/tree/sys/kern/sched_ule.c#n2986

(edit: looking at it again, I think I missed something still... it made a lot more sense in the middle of the night)

In D32797#740185, @kevans wrote:

In D32797#740166, @markj wrote:

I'm sorry, but I still don't understand the problem. Which scheduler data structures are not initialized at SI_SUB_SMP? Note that "at init_secondary() time" is ambiguous: APs are started fairly early during boot, but they won't call cpu_initclocks_ap() until aps_ready is set, which happens at SI_SUB_SMP, i.e., quite late, and a point where it's perfectly safe to schedule other threads. But we shouldn't be doing that unless the idlethread's spinlock count drops to 0, which shouldn't happen until we reach fork_exit().

Sorry, the description ended up being really bad/inaccurate. I'll rewrite it, but here's the breakdown:

• init_secondary() -> cpu_initclocks_ap() -> handleevents() -> callout_process() -> swi_sched() -> intr_event_schedule_thread() () -> sched_add() -> ... -> sched_setpreempt() -> asserts on curthread->td_lock *before* init_secondary has called sched_throw(NULL)
• curthread == idlethread at that time
• idlethread->td_lock isn't setup until later, at https://cgit.freebsd.org/src/tree/sys/kern/sched_ule.c#n2986

(edit: looking at it again, I think I missed something still... it made a lot more sense in the middle of the night)

That's interesting. A couple of observations:

• init_secondary() sets curthread to the AP's idle thread. The idle threads are created in idle_setup(). When creating a new thread, we _do_set td_lock, in sched_fork_thread(). So init_secondary() should be running with an initialized td_lock. But, it's the wrong lock initially: ULE uses per-CPU locks and the new thread's lock is inherited from the creating thread's. When the BSP creates idle threads, they'll all have td_lock set to CPU 0's lock. This gets corrected later as you noted, but not soon enough.
• Nothing actually acquires curthread's lock in the call path you described. The code is assuming that curthread's td_lock is the same as td's td_lock. Note in particular that sched_pickcpu() will just return the current CPU since smp_started is still false when cpu_initclocks_ap() is called. So there is a window where curthread's td_lock is wrong, and that violates the assumption of sched_add().

So I guess the question is, do we want to fix up scheduler state earlier in init_secondary(), or go with an approach like the one in this patch?

In D32797#740186, @markj wrote:

In D32797#740185, @kevans wrote:

In D32797#740166, @markj wrote:

I'm sorry, but I still don't understand the problem. Which scheduler data structures are not initialized at SI_SUB_SMP? Note that "at init_secondary() time" is ambiguous: APs are started fairly early during boot, but they won't call cpu_initclocks_ap() until aps_ready is set, which happens at SI_SUB_SMP, i.e., quite late, and a point where it's perfectly safe to schedule other threads. But we shouldn't be doing that unless the idlethread's spinlock count drops to 0, which shouldn't happen until we reach fork_exit().

Sorry, the description ended up being really bad/inaccurate. I'll rewrite it, but here's the breakdown:

• init_secondary() -> cpu_initclocks_ap() -> handleevents() -> callout_process() -> swi_sched() -> intr_event_schedule_thread() () -> sched_add() -> ... -> sched_setpreempt() -> asserts on curthread->td_lock *before* init_secondary has called sched_throw(NULL)
• curthread == idlethread at that time
• idlethread->td_lock isn't setup until later, at https://cgit.freebsd.org/src/tree/sys/kern/sched_ule.c#n2986

(edit: looking at it again, I think I missed something still... it made a lot more sense in the middle of the night)

That's interesting. A couple of observations:

• init_secondary() sets curthread to the AP's idle thread. The idle threads are created in idle_setup(). When creating a new thread, we _do_set td_lock, in sched_fork_thread(). So init_secondary() should be running with an initialized td_lock. But, it's the wrong lock initially: ULE uses per-CPU locks and the new thread's lock is inherited from the creating thread's. When the BSP creates idle threads, they'll all have td_lock set to CPU 0's lock. This gets corrected later as you noted, but not soon enough.
• Nothing actually acquires curthread's lock in the call path you described. The code is assuming that curthread's td_lock is the same as td's td_lock. Note in particular that sched_pickcpu() will just return the current CPU since smp_started is still false when cpu_initclocks_ap() is called. So there is a window where curthread's td_lock is wrong, and that violates the assumption of sched_add().

So I guess the question is, do we want to fix up scheduler state earlier in init_secondary(), or go with an approach like the one in this patch?

Let's pull in, maybe, @jhb and @kib as well, since other platforms (including x86) with EARLY_AP_STARTUP unset are also open to it.

Adding a sched_init_ap() that basically does the td == NULL branch (leaving the td != NULL branch) of sched_throw() seems like it'd be sufficient and, if I'm understanding this right, allow us to just handle the pending nvme callout without an initial trip through the idle thread? That'd be less ugly, too.

In D32797#740218, @kevans wrote:

Adding a sched_init_ap() that basically does the td == NULL branch (leaving the td != NULL branch) of sched_throw() seems like it'd be sufficient and, if I'm understanding this right, allow us to just handle the pending nvme callout without an initial trip through the idle thread? That'd be less ugly, too.

Sorry, last comment for now... this also does what I want, if that's a little more palatable: https://people.freebsd.org/~kevans/sched.diff -- then we have no problem scheduling the callout and it's a little more elegant, while being much less invasive looking. Only arm64 done here, but I can fix up the other archs if that's the route we want to go.

In D32797#740258, @kevans wrote:

In D32797#740218, @kevans wrote:

Adding a sched_init_ap() that basically does the td == NULL branch (leaving the td != NULL branch) of sched_throw() seems like it'd be sufficient and, if I'm understanding this right, allow us to just handle the pending nvme callout without an initial trip through the idle thread? That'd be less ugly, too.

Sorry, last comment for now... this also does what I want, if that's a little more palatable: https://people.freebsd.org/~kevans/sched.diff -- then we have no problem scheduling the callout and it's a little more elegant, while being much less invasive looking. Only arm64 done here, but I can fix up the other archs if that's the route we want to go.

I like this approach. In general I would put this call immediately after the code in init_secondary() which sets curthread.

If you are adding the stuff to be called from initsecondary(), can we try to remove td == NULL code from sched_throw() at all, moving it all to schedinit_ap()?

In D32797#740443, @kib wrote:

If you are adding the stuff to be called from initsecondary(), can we try to remove td == NULL code from sched_throw() at all, moving it all to schedinit_ap()?

We can't correct the spinlock nesting from schedinit_ap() because we can't hold the sched lock from there all the way to sched_throw() without recursing on it in between if there's a callout pending. We can't really drop it completely or anything that uses spinlocks between the two risks being preempted prematurely upon dropping the lock. =(