|Summary:||[patch] [sched] Fix comment in sched_balance_pair and move load check ahead of lock|
|Product:||Base System||Reporter:||Harrison Grundy <harrison.grundy>|
|Component:||kern||Assignee:||freebsd-bugs (Nobody) <bugs>|
|Status:||Closed Overcome By Events|
|Severity:||Affects Only Me||CC:||current, emaste, fidaj, jeff, kevans, koobs, markj|
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
Comment 6 Kyle Evans 2020-03-22 21:49:46 UTC
CC'ing Jeff, maybe
Comment 7 Mark Johnston 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 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 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 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.