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. > > 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 > > 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. Regards, Vincent > > I would vote for removing this SCHED_LOAD_RESOLUTION thing completely so that > we can > assume that load/util and capacity are always using 1024/10. > > Cheers, > > -- Dietmar > >> >> >> Regards, >> Vincent >> >> >>> >>> LOAD (UTIL) and CAPACITY have the same SCALE and SHIFT values because >>> SCHED_LOAD_RESOLUTION is always defined to 0. scale_load() and >>> scale_load_down() are also NOPs so this area is probably >>> worth a separate clean-up. >>> Beyond that, I'm not sure if the current functionality is >>> broken if we use different SCALE and SHIFT values for LOAD and CAPACITY? >>> > > [...] > -- 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/
