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 Regards, Vincent > > 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/
