Messages in this thread | | | Date | Tue, 15 Feb 2011 10:26:01 -0800 | Subject | Re: [PATCH] sched: Wholesale removal of sd_idle logic | From | Venkatesh Pallipadi <> |
| |
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |