Bug 197923 - [patch] [sched] Fix comment in sched_balance_pair and move load check ahead of lock
Summary: [patch] [sched] Fix comment in sched_balance_pair and move load check ahead o...
Status: Closed Overcome By Events
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL: https://github.com/AstrodogInc/freebs...
Keywords: needs-qa, patch
Depends on:
Reported: 2015-02-22 15:40 UTC by Harrison Grundy
Modified: 2020-04-03 14:25 UTC (History)
7 users (show)

See Also:

Fix comment in sched_balance_pair and move load check ahead of lock (928 bytes, patch)
2015-02-22 15:40 UTC, Harrison Grundy
no flags Details | Diff
Updated Patch (929 bytes, patch)
2015-02-22 16:53 UTC, Harrison Grundy
no flags Details | Diff
Thanks, Mark! Fixed Typo. (958 bytes, patch)
2020-03-22 22:20 UTC, Harrison Grundy
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Harrison Grundy 2015-02-22 15:40:48 UTC
Created attachment 153318 [details]
Fix comment in sched_balance_pair and move load check ahead of lock

Comment in sched_balance_pair suggests that it may balance more than one thread at a time, while tdq_move operates on only one thread.

Also move load check to before tdq_lock_pair, as the tdq lock is not required to
check load.
Comment 1 Ivan Klymenko 2015-02-22 16:28:55 UTC
(In reply to Harrison Grundy from comment #0)
-	if (high->tdq_load <= low->tdq_load)
-		return(0)
+	if (high->tdq_load <= low->tdq_load)
+		return(0);
Comment 2 Harrison Grundy 2015-02-22 16:53:01 UTC
Created attachment 153321 [details]
Updated Patch

Thanks Ivan.
Comment 3 Ivan Klymenko 2015-02-23 13:04:10 UTC
Maybe still "high-> tdq_transferable! = 0" because "tdq_transferable - transferable thread count". And if the counter is already 0, then the thread can not be transferable?
Comment 4 Harrison Grundy 2015-02-23 13:09:25 UTC
It still checks that in the lower if. We can't check tdq_transferable without the lock.
Comment 5 Harrison Grundy 2020-03-22 21:48:05 UTC
Still applies...
Comment 6 Kyle Evans freebsd_committer 2020-03-22 21:49:46 UTC
CC'ing Jeff, maybe
Comment 7 Mark Johnston freebsd_committer 2020-03-22 22:19:17 UTC
The comment was corrected in r329844.  I think the rest of the patch is not correct: high->tdq_transferable == 0 implies that the run queue contains no thread that can migrate, so tdq_move() will always fail.  We also should perform the queue load comparison with the queue locks held, since the situation may change after the queues are locked and we might end up migrating a thread from a less-loaded queue to a more-loaded queue, which defeats the goal of the code.  sched_balance_pair()'s caller already does the work of locklessly finding a pair of unbalanced queues.
Comment 8 Harrison Grundy 2020-03-22 22:20:20 UTC
Created attachment 212626 [details]
Thanks, Mark! Fixed Typo.
Comment 9 Harrison Grundy 2020-03-22 22:25:26 UTC
If the pair is already identified, is there a need to do this check?
Comment 10 Mark Johnston freebsd_committer 2020-03-22 22:34:25 UTC
(In reply to Harrison Grundy from comment #9)
Yes, the load values are not stable unless the corresponding queues are locked, so they may be out of date.
Comment 11 Harrison Grundy 2020-03-22 22:42:02 UTC
(In reply to Mark Johnston from comment #10)

Right, but it seems like locking the tdq to "force" them into alignment would have a bigger impact on performance than occassionally mis-balancing. If we take the lock, and push a new thread onto the target, or pull one from the source, when they're waiting on the lock somewhere else, the end result would still be incorrect, wouldn't it?
Comment 12 Mark Johnston freebsd_committer 2020-03-22 22:59:12 UTC
(In reply to Harrison Grundy from comment #11)
It is possible that the queues will end up unbalanced again immediately after they are unlocked, but sched_balance_pair() has to lock the queues to perform the migration anyway, so it might as well cheaply verify that it's doing the right thing before performing an expensive migration.
Comment 13 Mark Johnston freebsd_committer 2020-04-03 14:25:50 UTC
I'm going to close this: the comment is already fixed, and I think the current logic for comparing queue loads is sound.