On 08/09/15 15:01, Vincent Guittot wrote: > On 8 September 2015 at 14:50, Dietmar Eggemann <dietmar.eggem...@arm.com> > wrote: >> 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. > > If you set 2 different values to SCHED_LOAD_SHIFT and > SCHED_CAPACITY_SHIFT for test purpose, you will see that util_avg will > not use the right range of value > > If we don't take into account freq and cpu invariance in a 1st step > > sa->util_sum is a load in the range [0..LOAD_AVG_MAX]. I say load > because of the max value > > the current implementation of util_avg is > sa->util_avg = (sa->util_sum << SCHED_LOAD_SHIFT) / LOAD_AVG_MAX > > so sa->util_avg is a load in the range [0..SCHED_LOAD_SCALE] > > the current implementation of get_cpu_usage is > return (sa->util_avg * capacity_orig_of(cpu)) >> SCHED_LOAD_SHIFT > > so the usage has the same unit and range as capacity of the cpu and > can be compared with another capacity value > > Your patchset returns directly sa->util_avg which is a load to compare > it with capacity value > > So you have to convert sa->util_avg from load to capacity so if you have > sa->util_avg = (sa->util_sum << SCHED_CAPACITY_SHIFT) / LOAD_AVG_MAX > > sa->util_avg is now a capacity with the same range as you cpu thanks > to the cpu invariance factor that the patch 3 has added. > > the << SCHED_CAPACITY_SHIFT above can be optimized with the >> > SCHED_CAPACITY_SHIFT included in > sa->util_sum += scale(contrib, scale_cpu); > as mentioned by Peter > > At now, SCHED_CAPACITY_SHIFT is set to 10 as well as SCHED_LOAD_SHIFT > so using one instead of the other doesn't change the result but if > it's no more the case, we need to take care of the range/unit that we > use
No arguing here, I just called this a SHIFT/SCALE problem. [...] -- 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/
