Page MenuHomeFreeBSD

Do less sleepqueue processing post-switch so that we can avoid holding thethread lock.
ClosedPublic

Authored by jeff on Dec 10 2019, 4:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 20 2023, 4:30 AM
Unknown Object (File)
Dec 3 2023, 7:47 AM
Unknown Object (File)
Nov 14 2023, 4:41 PM
Unknown Object (File)
Oct 13 2023, 3:40 PM
Unknown Object (File)
Sep 15 2023, 11:19 AM
Unknown Object (File)
Sep 7 2023, 8:31 AM
Unknown Object (File)
Sep 7 2023, 8:29 AM
Unknown Object (File)
Sep 7 2023, 8:28 AM
Subscribers

Details

Summary

Various restructuring has made it possible to do all post-wakeup processing in sleepq_resume_thread() where the thread lock is already held. This will permit a follow-up commit where sched_switch() does not return with the thread lock held. I also believe the resulting code looks cleaner.

This has passed stress2

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28054
Build 26202: arc lint + arc unit

Event Timeline

jeff retitled this revision from Do less sleepqueue processing post-switch so that we can avoid holding the thread lock. to Do less sleepqueue processing post-switch so that we can avoid holding thethread lock..Dec 10 2019, 4:07 AM
jeff edited the summary of this revision. (Show Details)
jeff added reviewers: jhb, kib, mav.
jeff set the repository for this revision to rS FreeBSD src repository - subversion.

A few small suggestions.

sys/kern/subr_sleepqueue.c
384

Shouldn't this always be clear at this point?

638

We didn't used to have the td_sleeptimo check and that is when some of this became redundant.

686

Now that sleepq_check_signals() doesn't do any real work, we can make this a bit simpler perhaps:

if (rcatch)
    return (rcatch);
return (td->td_intrval);

This would also mean just axing sleepq_check_signals() entirely.

A futher refinement would be to change sleepq_catch_signals to set td_intrval directly instead of 'ret =' for the non-zero return values and to change it to return void. You can then remove 'rcatch' and all the 'if (rcatch)' checks.

707

Similarly, axe rval and just use:

return (sleepq_check_timeout());
722

This one is the only one still somewhat complex:

thread_unlock(curthread);
rvalt = sleepq_check_timeout();

if (rcatch)
    return (rcatch);
if (td->td_intrval)
    return (td->td_intrval)
return (rvalt);
1056

This comment is kind of wrong. What this case is handling is when the callout_stop failed to cancel the callout and it is running spuriously after the thread has already resumed.

1058

This can probably just be TD_IS_SLEEPING? You can't be sleeping and not be on a sleepq.

1060

This comment is also stale now and probably can just be axed.

This revision is now accepted and ready to land.Dec 11 2019, 7:46 PM
sys/kern/subr_sleepqueue.c
384

This was just defensive. I can make it an assert.

686

Yeah I can simplify the control flow here. I like keeping the function though as it's a good reminder of what it's for.

sys/kern/subr_sleepqueue.c
868

Doesn't this recurse into the sleepq code and establish an order between sleepq locks ? At least if we raced.

sys/kern/subr_sleepqueue.c
868

This was a good question which I had not considered as I forgot about the sleepq relationship with callout. In any event, we do not provide a lock and we do not call with DRAIN so no sleepq locks, our own or otherwise, are inspected or acquired in callout_stop(). We simply mark the thread and the cpu executing the callout will resume once we unblock the lock and discover that the callout is no longer scheduled.

The patch looks good, modulo John' notes.

sys/kern/subr_sleepqueue.c
868

I see, perpahs a comment should also note this.

This revision was automatically updated to reflect the committed changes.
jeff added a commit: rS355781: schedlock 2/4.