Eliminate recursion from most thread_lock() consumers. Allow sched_add() and all callers to return without the thread_lock held.

[jeff]

Summary

After this patch the set gets smaller. This is unfortunately quite broad. This is part of a set from https://github.com/freebsd/freebsd/compare/master...jwroberson:schedlock

The impetus is to do lockless tdq_add on remote cpus. That is not part of this patch or branch. Eliminating recursion also greatly reduces the number of atomics in critical scheduler paths. This adds two thread flags that could possibly use better names. I did not want to add HOLDTD but I could not see a way to implement priority lending cleanly without it. While the signal code would be simpler if I used it, it would make fewer cases where lockless add would be profitable and longer code chains with sched lock still held.

The sleepq_remove_thread and weed_inhib changes need the most attention. The rest is really boilerplate.

Test Plan

Still running stress2

[kib]

sys/kern/kern_clock.c

286 ↗

(On Diff #65107)

Why this part of the condition is removed ?

sys/kern/kern_sig.c

2524 ↗

(On Diff #65107)

All changes in sig_suspend_threads() are purely stylistic. Am I right ?

If you really want/need them, please commit separately.

sys/kern/kern_synch.c

574 ↗

(On Diff #65107)

But if the inhibitor is not swapout, then why don't you do sched_wakeup() as before ?

[jeff]

sys/kern/kern_clock.c

286 ↗

(On Diff #65107)

ON_SLEEPQ is not yet sleeping. It would mean that the thread lock was dropped and we were preempted. To prevent recursion on the sleepq lock here I would have to detect whether td_lock is the sleepq lock yet before calling sleepq_type(). It doesn't seem necessary.

sys/kern/kern_sig.c

2524 ↗

(On Diff #65107)

The continue is important.

Likely the block below that I originally changed and then reverted the functional diff without noticing I had changed style.

sys/kern/kern_synch.c

574 ↗

(On Diff #65107)

If TDI_SWAPPED was not the only inhibitor we returned immediately. You can't sched_add an inhibited thread. Part of the reason I restructured the code beyond my immediate requirement is that it was unnecessarily difficult to follow in that case.

[mav]

sys/dev/ocs_fc/ocs_os.c

662 ↗

(On Diff #65107)

This function seems dead, but if leave it for later, since the driver never calls thread_lock(), I think it could have some more sense to add thread lock before the call rather then add the flag.

[jhb]

I think this looks ok overall.

sys/dev/ocs_fc/ocs_os.c

662 ↗

(On Diff #65107)

I suspect @mav is right here that the existing version is just busted and missing thread_lock entirely, so you could add the missing lock and let sched_add() drop it as in other callers.

sys/kern/kern_clock.c

286 ↗

(On Diff #65107)

Agreed that TD_ON_SLEEPQ should be redundant here and not needed.

sys/kern/kern_kthread.c

147 ↗

(On Diff #65107)

Maybe write this without the negation now that there's an else clause:

if (flags & RF_STOPPED)
   thread_unlock();
else
   sched_add(td, SRQ_BORING);

sys/kern/kern_sig.c

2237 ↗

(On Diff #65107)

Nit: blank line before block comment below.

sys/kern/kern_synch.c

562 ↗

(On Diff #65107)

Maybe s/handles/Drops/?

568 ↗

(On Diff #65107)

S/arange/arrange/ (old typo) while you are here.

sys/kern/subr_sleepqueue.c

770 ↗

(On Diff #65107)

Hmm, so are callers supposed to always have the sc locked when this is called now? If so, should we assert that?

808 ↗

(On Diff #65107)

I misparsed this logic the first time and thought the 'srqflags' check below was redundant. It might be clearer if you set 'drop' to false here and drop the else clause.

838 ↗

(On Diff #65107)

Nit: ()'s around setrunnable.

[jeff]

sys/kern/subr_sleepqueue.c

770 ↗

(On Diff #65107)

It is asserted immediately in lookup. To assert it myself I have to replicate most of the code in lookup anyhow.

[jeff]

Address review feedback.

[jhb]

sys/kern/subr_sleepqueue.c

770 ↗

(On Diff #65107)

Ah, ok, works for me then.