On 08/09/15 08:22, Vincent Guittot wrote: > On 7 September 2015 at 20:54, Dietmar Eggemann <dietmar.eggem...@arm.com> > wrote: >> On 07/09/15 17:21, Vincent Guittot wrote: >>> On 7 September 2015 at 17:37, Dietmar Eggemann <dietmar.eggem...@arm.com> >>> 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 [...] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
