Hi On Tue, Apr 13, 2021 at 1:15 AM Pierre Gondois <pierre.gond...@arm.com> wrote: > > 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. In fair.c:select_task_rq_fair(), when feec() returns a error (< 0), the new_cpu = find_idlest_cpu() or select_idle_sibling(). In your patch, you should return prev_cpu instead of -1 if you want to select the prev_cpu.
> Are you planning to submit a V2 with the bail out mechanism ? Maybe it would be better if you submitted the V2 ? I just check the patch and raised some questions. > Regards