Re: [PATCH] sched: Wholesale removal of sd_idle logic

[Venkatesh Pallipadi <venki@google.com>]

On Tue, Feb 15, 2011 at 9:01 AM, Vaidyanathan Srinivasan
<svaidy@linux.vnet.ibm.com> wrote:
> * Venkatesh Pallipadi <venki@google.com> [2011-02-14 14:38:50]:
>
>> sd_idle logic was introduced way back in 2005 (commit 5969fe06),
>> as an HT optimization.
>>
>> As per the discussion in the thread here
>> lkml subject - sched: Resolve sd_idle and first_idle_cpu Catch-22 - v1
>> https://patchwork.kernel.org/patch/532501/
>>
>> the capacity based logic in the load balancer right now handles this
>> in a much cleaner way, handling more than 2 SMT siblings etc, and sd_idle
>> does not seem to bring any adiitional benefits. sd_idle logic also has
>> some bugs that has performance impact. Here is the patch that removes
>> the sd_idle logic altogether.
>>
>> The initial patch here - https://patchwork.kernel.org/patch/532501/
>> applies cleanly over the below change and provides a micro-optimization
>> for a specific case, where an idle core can pull tasks instead of a
>> core with one thread being idle and other thread being busy.
>> It will be good to get some data on whether this micro-optimization
>> matters or not.
>>
>> Also, there was a dependency of sched_mc_power_savings == 2, with sd_idle
>> logic. Copying Vaidy to know the impact of this change there.
>
> Hi Venki,
>
> The dependency is to avoid active balancing when there is a busy
> sibling and power save balance is not set.
>
> Another logic would propagate/force sd_idle=1 to induce more frequent
> balancing for idle sibling in case of power save balance.  Removing
> sd_idle will make this default.
>
> Your changes look good.  I will test and report.
>
>> Signed-off-by: Venkatesh Pallipadi <venki@google.com>
>
> Acked-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
>
>> ---
>>  kernel/sched_fair.c |   53 ++++++++++----------------------------------------
>>  1 files changed, 11 insertions(+), 42 deletions(-)
>>

<snip>

>> @@ -3386,10 +3363,6 @@ redo:
>>                       sd->balance_interval *= 2;
>>       }
>>
>> -     if (!ld_moved && !sd_idle && sd->flags & SD_SHARE_CPUPOWER &&
>> -         !test_sd_parent(sd, SD_POWERSAVINGS_BALANCE))
>> -             ld_moved = -1;
>
> I have not figured out where ld_moved is checked for -1 and why we
> need to treat this as a special case.
>

Return value of -1 was being consumed in rebalance domains() call
to load_balance(). Returning -1 (instead of 0 in this case) makes
rebalance_domains() to call higher domain load balancing
with CPU_NOT_IDLE, when sibling is busy and even when there
was no load pulled in.

Thanks,
Venki

> Your bug fix in idle_balance() for if (pulled_task) {...} is a good
> catch.
>
>> -
>>       goto out;
>>
>>  out_balanced:
>> @@ -3403,11 +3376,7 @@ out_one_pinned:
>>                       (sd->balance_interval < sd->max_interval))
>>               sd->balance_interval *= 2;
>>
>> -     if (!sd_idle && sd->flags & SD_SHARE_CPUPOWER &&
>> -         !test_sd_parent(sd, SD_POWERSAVINGS_BALANCE))
>> -             ld_moved = -1;
>> -     else
>> -             ld_moved = 0;
>
> Ack.  But why did we have to flag this case earlier?
>
>> +     ld_moved = 0;
>>  out:
>>       return ld_moved;
>>  }
>
> --Vaidy
>
>