Page MenuHomeFreeBSD

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

Authored by jeff on Sun, Dec 1, 8:50 PM.

Details

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

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 28034
Build 26185: arc lint + arc unit

Event Timeline

jeff created this revision.Sun, Dec 1, 8:50 PM
jeff retitled this revision from Eliminate recursion from most thread_lock() consumers. Allow sched_add() and all callers to return without the thread_lock held. to Eliminate recursion from most thread_lock() consumers. Allow sched_add() and all callers to return without the thread_lock held..Sun, Dec 1, 8:59 PM
jeff edited the summary of this revision. (Show Details)
jeff edited the test plan for this revision. (Show Details)
jeff added reviewers: mav, kib, markj, jhb, mjg.
kib added inline comments.Sun, Dec 1, 9:18 PM
sys/kern/kern_clock.c
286

Why this part of the condition is removed ?

sys/kern/kern_sig.c
2534–2535

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

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

jeff added inline comments.Sun, Dec 1, 9:28 PM
sys/kern/kern_clock.c
286

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
2534–2535

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

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 added inline comments.Mon, Dec 2, 12:03 AM
sys/dev/ocs_fc/ocs_os.c
664

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 added a comment.Fri, Dec 6, 7:09 PM

I think this looks ok overall.

sys/dev/ocs_fc/ocs_os.c
664

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

Agreed that TD_ON_SLEEPQ should be redundant here and not needed.

sys/kern/kern_kthread.c
147

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
2247

Nit: blank line before block comment below.

sys/kern/kern_synch.c
562

Maybe s/handles/Drops/?

568

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

sys/kern/subr_sleepqueue.c
770–771

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

808

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

Nit: ()'s around setrunnable.

jeff added inline comments.Sun, Dec 8, 2:01 AM
sys/kern/subr_sleepqueue.c
770–771

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

jeff updated this revision to Diff 65394.Sun, Dec 8, 2:03 AM

Address review feedback.

jhb accepted this revision.Mon, Dec 9, 6:06 PM
jhb added inline comments.
sys/kern/subr_sleepqueue.c
770–771

Ah, ok, works for me then.

This revision is now accepted and ready to land.Mon, Dec 9, 6:06 PM