Bug 223914 - [PATCH] Fix bug in which the long term ULE load balancer is executed only once
Summary: [PATCH] Fix bug in which the long term ULE load balancer is executed only once
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: FreeBSD bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-11-27 17:51 UTC by Justinien Bouron
Modified: 2017-12-12 20:53 UTC (History)
5 users (show)

See Also:


Attachments
Patch for the ULE load balancer solving the issue of this bug report. (462 bytes, patch)
2017-11-27 17:51 UTC, Justinien Bouron
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justinien Bouron 2017-11-27 17:51:02 UTC
Created attachment 188323 [details]
Patch for the ULE load balancer solving the issue of this bug report.

The load balancer of ULE has a bug which cause it to be executed only once at startup, and it will *never* be executed again.

The reason is simple, when the sched_balance function is called for the first time, the smp is not yet setup, and the function returns.
However the balance_tick variable is not resetted before the return and thus will remains 0 for ever.

Because the condition in sched_clock to trigger the load balancer is :
" balance_ticks && --balance_ticks == 0 ", once balance_ticks is 0, this condition will never be met, and thus the load balancer will never be executed again.

To convince yourself you can even put a panic right before the call to sched_balance_group in sched_balance, it will never panic ...

The fix is rather simple, just move the line resetting the balance_ticks value *before* the if branch "if (smp_started == 0 || rebalance == 0)". That way we are sure that the load balancer is re-scheduled.
Comment 1 Konstantin Belousov freebsd_committer 2017-11-28 18:36:27 UTC
I think the analysis is correct.

Might be, sched_initticks() should set the balance_ticks as well, after the balance_interval is initialized ?
Comment 2 Justinien Bouron 2017-11-28 20:51:23 UTC
sched_setup calls sched_setup_smp which in turns calls sched_balance that will set the balance_ticks.
Comment 3 Ivan Klymenko 2017-12-12 11:47:46 UTC
Hi.
Maybe, this version use less overhead charges for not SMP and given the sched_random() function.

--- sched_ule.c.orig    2017-11-29 09:23:11.503091000 +0200
+++ sched_ule.c.my1     2017-12-12 13:40:46.330506000 +0200
@@ -880,11 +880,15 @@
 {
        struct tdq *tdq;
 
-       if (smp_started == 0 || rebalance == 0)
+       if (smp_started == 0)
                return;
 
        balance_ticks = max(balance_interval / 2, 1) +
            (sched_random() % balance_interval);
+
+       if (rebalance == 0)
+               return;
+
        tdq = TDQ_SELF();
        TDQ_UNLOCK(tdq);
        sched_balance_group(cpu_top);
Comment 4 Ivan Klymenko 2017-12-12 20:53:13 UTC
(In reply to Ivan Klymenko from comment #3)
Sorry, I think I said foolishness.