Re: [PATCH] sched/fair: use signed long when compute energy delta in eas

[Pierre Gondois <pierre.gondois@arm.com>]

Hi
> > > >
> > > > This patch-set is not significantly improving the execution time of
> > > > feec(). The results we have so far are an improvement of 5-10% in
> > > > execution time, with feec() being executed in < 10us. So the 
> gain is not
> > > > spectacular.
> > >
> > > well， I meaned to cache all util value and compute energy with 
> caches,
> > > when
> > > (cpu==dst_cpu), use caches instead of updating util, and do not get
> > > util with function:
> > >  "effective_cpu_util()", to compute util with cache.
> > > I add more parameters into pd_cache:
> > > struct pd_cache {
> > >         unsigned long util;
> > >         unsigned long util_est;
> > >         unsigned long util_cfs;
> > >         unsigned long util_irq;
> > >         unsigned long util_rt;
> > >         unsigned long util_dl;
> > >         unsigned long bw_dl;
> > >         unsigned long freq_util;
> > >         unsigned long nrg_util;
> > > };
> > > In this way, it can avoid util update while feec. I tested with it,
> > > and the negative delta disappeared.
> > > Maybe this is not a good method, but it does work.
> > If I understand correctly, you put all the fields used by
> > core.c:effective_cpu_util() in the caches, allowing to have values not
> > subject to updates.
> Yes.
> > core.c:effective_cpu_util() isn't only called from
> > fair.c:compute_energy(). It is used in the cpufreq_schedutil.c and
> > cpufreq_cooling.c (through core.c:sched_cpu_util()).
> > Did you have to duplicate core.c:effective_cpu_util() to have a second
> > version using the caches ? If yes, I think the function was meant to be
> > unique so that all the utilization estimations go through the same path.
> >
> I defined a new function to distinguish it from the effective_cpu_util.
>
> > If your concern is to avoid negative delta, I think just bailing out
> > when this happens should be sufficient. As shown in the last message,
> > having a wrong placement should not happen that often, plus the prev_cpu
> > should be used which should be ok.
> In your patch, you didn't actually choose the prev_cpu. you return (-1);
>
> > If you want to cache the values, I think a stronger justification will
> > be asked: this seems to be a big modification compared to the initial
> > issue, knowing that another simpler solution is available (i.e. bailing
> > out). I was not able to prove there was a significant gain in the
> > find_energy_efficient_cpu() execution time, but I would be happy if you
> > can, or if you find other arguments.
> Yes, you are right, perhaps there is indeed no need for such a big 
> modification.
>
> Regards

In fair.c:select_task_rq_fair(), if feec() returns a error (< 0), then 
prev_cpu is selected. I think it's better to still let feec() signal 
that something happened and let select_task_rq_fair() select prev_cpu by 
itself.
Are you planning to submit a V2 with the bail out mechanism ?

Regards