From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754690AbbIHMuo (ORCPT ); Tue, 8 Sep 2015 08:50:44 -0400 Received: from eu-smtp-delivery-143.mimecast.com ([207.82.80.143]:31566 "EHLO eu-smtp-delivery-143.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753367AbbIHMum convert rfc822-to-8bit (ORCPT ); Tue, 8 Sep 2015 08:50:42 -0400 Subject: Re: [PATCH 5/6] sched/fair: Get rid of scaling utilization by capacity_orig To: Vincent Guittot References: <1439569394-11974-1-git-send-email-morten.rasmussen@arm.com> <1439569394-11974-6-git-send-email-morten.rasmussen@arm.com> <55E8DD00.2030706@linaro.org> <55EDAF43.30500@arm.com> <55EDDD5A.70904@arm.com> Cc: Steve Muckle , Morten Rasmussen , "peterz@infradead.org" , "mingo@redhat.com" , "daniel.lezcano@linaro.org" , "yuyang.du@intel.com" , "mturquette@baylibre.com" , "rjw@rjwysocki.net" , Juri Lelli , "sgurrappadi@nvidia.com" , "pang.xunlei@zte.com.cn" , "linux-kernel@vger.kernel.org" From: Dietmar Eggemann Message-ID: <55EED99E.2040100@arm.com> Date: Tue, 8 Sep 2015 13:50:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: X-OriginalArrivalTime: 08 Sep 2015 12:50:39.0021 (UTC) FILETIME=[F699ADD0:01D0EA34] X-MC-Unique: UOOErf7HTf6HBeM6vUpomw-1 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/09/15 08:22, Vincent Guittot wrote: > On 7 September 2015 at 20:54, Dietmar Eggemann wrote: >> On 07/09/15 17:21, Vincent Guittot wrote: >>> On 7 September 2015 at 17:37, Dietmar Eggemann wrote: >>>> On 04/09/15 00:51, Steve Muckle wrote: >>>>> Hi Morten, Dietmar, >>>>> >>>>> On 08/14/2015 09:23 AM, Morten Rasmussen wrote: >>>>> ... >>>>>> + * cfs_rq.avg.util_avg is the sum of running time of runnable tasks plus the >>>>>> + * recent utilization of currently non-runnable tasks on a CPU. It represents >>>>>> + * the amount of utilization of a CPU in the range [0..capacity_orig] where >>>>> >>>>> I see util_sum is scaled by SCHED_LOAD_SHIFT at the end of >>>>> __update_load_avg(). If there is now an assumption that util_avg may be >>>>> used directly as a capacity value, should it be changed to >>>>> SCHED_CAPACITY_SHIFT? These are equal right now, not sure if they will >>>>> always be or if they can be combined. >>>> >>>> You're referring to the code line >>>> >>>> 2647 sa->util_avg = (sa->util_sum << SCHED_LOAD_SHIFT) / LOAD_AVG_MAX; >>>> >>>> in __update_load_avg()? >>>> >>>> Here we actually scale by 'SCHED_LOAD_SCALE/LOAD_AVG_MAX' so both values are >>>> load related. >>> >>> I agree with Steve that there is an issue from a unit point of view >>> >>> sa->util_sum and LOAD_AVG_MAX have the same unit so sa->util_avg is a >>> load because of << SCHED_LOAD_SHIFT) >>> >>> Before this patch , the translation from load to capacity unit was >>> done in get_cpu_usage with "* capacity) >> SCHED_LOAD_SHIFT" >>> >>> So you still have to change the unit from load to capacity with a "/ >>> SCHED_LOAD_SCALE * SCHED_CAPACITY_SCALE" somewhere. >>> >>> sa->util_avg = ((sa->util_sum << SCHED_LOAD_SHIFT) /SCHED_LOAD_SCALE * >>> SCHED_CAPACITY_SCALE / LOAD_AVG_MAX = (sa->util_sum << >>> SCHED_CAPACITY_SHIFT) / LOAD_AVG_MAX; >> >> I see the point but IMHO this will only be necessary if the SCHED_LOAD_RESOLUTION >> stuff gets re-enabled again. >> >> It's not really about utilization or capacity units but rather about using the same >> SCALE/SHIFT values for both sides, right? > > It's both a unit and a SCALE/SHIFT problem, SCHED_LOAD_SHIFT and > SCHED_CAPACITY_SHIFT are defined separately so we must be sure to > scale the value in the right range. In the case of cpu_usage which > returns sa->util_avg , it's the capacity range not the load range. Still don't understand why it's a unit problem. IMHO LOAD/UTIL and CAPACITY have no unit. I agree that with the current patch-set we have a SHIFT/SCALE problem once SCHED_LOAD_RESOLUTION is set to != 0. > >> >> I always thought that scale_load_down() takes care of that. > > AFAIU, scale_load_down is a way to increase the resolution of the > load not to move from load to capacity IMHO, increasing the resolution of the load is done by re-enabling this define SCHED_LOAD_RESOLUTION 10 thing (or by setting SCHED_LOAD_RESOLUTION to something else than 0). I tried to figure out why we have this issue when comparing UTIL w/ CAPACITY and not LOAD w/ CAPACITY: Both are initialized like that: sa->load_avg = scale_load_down(se->load.weight); sa->load_sum = sa->load_avg * LOAD_AVG_MAX; sa->util_avg = scale_load_down(SCHED_LOAD_SCALE); sa->util_sum = LOAD_AVG_MAX; and we use 'se->on_rq * scale_load_down(se->load.weight)' as 'unsigned long weight' argument to call __update_load_avg() making sure the scaling differences between LOAD and CAPACITY are respected while updating sa->load_sum (and sa->load_avg). OTAH, we don't apply a scale_load_down for sa->util_[sum/avg] only a '<< SCHED_LOAD_SHIFT) / LOAD_AVG_MAX' on sa->util_avg. So changing '<< SCHED_LOAD_SHIFT' to '* scale_load_down(SCHED_LOAD_SCALE)' would be the logical thing to do. I agree that '<< SCHED_CAPACITY_SHIFT' would have the same effect but why using a CAPACITY related thing on the LOAD/UTIL side? The only reason would be the unit problem which I don't understand. > >> >> So shouldn't: >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 3445d2fb38f4..b80f799aface 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -2644,7 +2644,7 @@ __update_load_avg(u64 now, int cpu, struct sched_avg *sa, >> cfs_rq->runnable_load_avg = >> div_u64(cfs_rq->runnable_load_sum, LOAD_AVG_MAX); >> } >> - sa->util_avg = (sa->util_sum << SCHED_LOAD_SHIFT) / LOAD_AVG_MAX; >> + sa->util_avg = (sa->util_sum * scale_load_down(SCHED_LOAD_SCALE)) / LOAD_AVG_MAX; >> } >> >> return decayed; >> >> fix that issue in case SCHED_LOAD_RESOLUTION != 0 ? > > > No, but > sa->util_avg = (sa->util_sum << SCHED_CAPACITY_SHIFT) / LOAD_AVG_MAX; > will fix the unit issue. > I agree that i don't change the result because both SCHED_LOAD_SHIFT > and SCHED_CAPACITY_SHIFT are set to 10 but as mentioned above, they > are set separately so it can make the difference if someone change one > SHIFT value. SCHED_LOAD_SHIFT and SCHED_CAPACITY_SHIFT can be set separately but the way to change SCHED_LOAD_SHIFT is by re-enabling the define SCHED_LOAD_RESOLUTION 10 in kernel/sched/sched.h. I guess nobody wants to change SCHED_CAPACITY_[SHIFT/SCALE]. Cheers, -- Dietmar [...]