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.
(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);
Created attachment 153321 [details] Updated Patch Thanks Ivan.
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?
It still checks that in the lower if. We can't check tdq_transferable without the lock.
Still applies...
CC'ing Jeff, maybe
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.
Created attachment 212626 [details] Thanks, Mark! Fixed Typo.
If the pair is already identified, is there a need to do this check?
(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.
(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?
(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.
I'm going to close this: the comment is already fixed, and I think the current logic for comparing queue loads is sound.