Messages in this thread | | | From | Xuewen Yan <> | Date | Tue, 25 Jul 2023 10:01:49 +0800 | Subject | Re: [PATCH] cpufreq: schedutil: next_freq need update when cpufreq_limits changed |
| |
Hi Pierre,
On Mon, Jul 24, 2023 at 11:33 PM Pierre Gondois <pierre.gondois@arm.com> wrote: > > > > On 7/24/23 05:36, Xuewen Yan wrote: > > On Sat, Jul 22, 2023 at 7:02 AM Qais Yousef <qyousef@layalina.io> wrote: > >> > >> On 07/19/23 21:05, Xuewen Yan wrote: > >>> When cpufreq's policy is single, there is a scenario that will > >>> cause sg_policy's next_freq to be unable to update. > >>> > >>> When the cpu's util is always max, the cpufreq will be max, > >>> and then if we change the policy's scaling_max_freq to be a > >>> lower freq, indeed, the sg_policy's next_freq need change to > >>> be the lower freq, however, because the cpu_is_busy, the next_freq > >>> would keep the max_freq. > >>> > >>> For example: > >>> The cpu7 is single cpu: > >>> > >>> unisoc:/sys/devices/system/cpu/cpufreq/policy7 # while true;do done& > >>> [1] 4737 > >>> unisoc:/sys/devices/system/cpu/cpufreq/policy7 # taskset -p 80 4737 > >>> pid 4737's current affinity mask: ff > >>> pid 4737's new affinity mask: 80 > >>> unisoc:/sys/devices/system/cpu/cpufreq/policy7 # cat scaling_max_freq > >>> 2301000 > >>> unisoc:/sys/devices/system/cpu/cpufreq/policy7 # cat scaling_cur_freq > >>> 2301000 > >>> unisoc:/sys/devices/system/cpu/cpufreq/policy7 # echo 2171000 > scaling_max_freq > >>> unisoc:/sys/devices/system/cpu/cpufreq/policy7 # cat scaling_max_freq > >>> 2171000 > >>> > >>> At this time, the sg_policy's next_freq would keep 2301000. > >>> > >>> To prevent the case happen, add the judgment of the need_freq_update flag. > >>> > >>> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com> > >>> Co-developed-by: Guohua Yan <guohua.yan@unisoc.com> > >>> Signed-off-by: Guohua Yan <guohua.yan@unisoc.com> > >>> --- > >>> kernel/sched/cpufreq_schedutil.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > >>> index 4492608b7d7f..458d359f5991 100644 > >>> --- a/kernel/sched/cpufreq_schedutil.c > >>> +++ b/kernel/sched/cpufreq_schedutil.c > >>> @@ -350,7 +350,8 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time, > >>> * Except when the rq is capped by uclamp_max. > >>> */ > >>> if (!uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu)) && > >>> - sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) { > >>> + sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq && > >>> + !sg_policy->need_freq_update) { > >> > >> What about sugov_update_single_perf()? It seems to have the same problem, no? > > > > There is no problem in sugov_update_single_perf, because the next_freq > > is updated by drivers, maybe the next_freq is not used when using > > sugov_update_single_perf.. > > > > But for the last_freq_update_time, I think there are some problems > > when using sugov_update_single_perf: > > Now, there is no judgment condition for the update of the > > last_freq_update_time. That means the last_freq_update_time is always > > updated in sugov_update_single_perf. > > And in sugov_should_update_freq: it would judge the > > freq_update_delay_ns. As a result, If we use the > > sugov_update_single_perf, the cpu frequency would only be periodically > > updated according to freq_update_delay_ns. > > Maybe we should judge the cpufreq_driver_adjust_perf's return value, > > if the freq is not updated, the last_freq_update_time also does not > > have to update. > > > > Just like: > > --- > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > > index 458d359f5991..10f18b054f01 100644 > > --- a/kernel/sched/cpufreq_schedutil.c > > +++ b/kernel/sched/cpufreq_schedutil.c > > @@ -381,6 +381,7 @@ static void sugov_update_single_perf(struct > > update_util_data *hook, u64 time, > > struct sugov_cpu *sg_cpu = container_of(hook, struct > > sugov_cpu, update_util); > > unsigned long prev_util = sg_cpu->util; > > unsigned long max_cap; > > + bool freq_updated; > > > > /* > > * Fall back to the "frequency" path if frequency invariance is not > > @@ -407,10 +408,11 @@ static void sugov_update_single_perf(struct > > update_util_data *hook, u64 time, > > sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util) > > sg_cpu->util = prev_util; > > > > - cpufreq_driver_adjust_perf(sg_cpu->cpu, map_util_perf(sg_cpu->bw_dl), > > + freq_updated = cpufreq_driver_adjust_perf(sg_cpu->cpu, > > map_util_perf(sg_cpu->bw_dl), > > map_util_perf(sg_cpu->util), max_cap); > > > > - sg_cpu->sg_policy->last_freq_update_time = time; > > + if (freq_updated) > > + sg_cpu->sg_policy->last_freq_update_time = time; > > } > > > > Hello Xuewen, > FWIW, the patch and explanation for sugov_update_single_perf() seem sensible to > me. Just a comment about cpufreq_driver_adjust_perf() and > (struct cpufreq_driver)->adjust_perf(): wouldn't their prototype need to be > updated (i.e. not return void) to do the change suggested above ?
Yes, their function type should be changed from void to bool or init. For this patch, I just raise a question for everyone to discuss. If this is a problem, the official patch needs to be revised later.
BR xuewen
> > Regards, > Pierre > > > > > BR > > Thanks! > > > > --- > > xuewen > >> > >> LGTM otherwise. > >> > >> > >> Cheers > >> > >> -- > >> Qais Yousef > >> > >>> next_f = sg_policy->next_freq; > >>> > >>> /* Restore cached freq as next_freq has changed */ > >>> -- > >>> 2.25.1 > >>> > >
| |