linux-kernel - Re: [PATCH] cpufreq: schedutil: Don't consider freq reduction to busy CPU if need_freq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210222170420.000019a3.zbestahu@163.com>
Date:   Mon, 22 Feb 2021 17:04:20 +0800
From:   Yue Hu <zbestahu@....com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Yue Hu <zbestahu@...il.com>, rjw@...ysocki.net, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, huyue2@...ong.com
Subject: Re: [PATCH] cpufreq: schedutil: Don't consider freq reduction to
 busy CPU if need_freq_update is set

On Mon, 22 Feb 2021 11:00:14 +0530
Viresh Kumar <viresh.kumar@...aro.org> wrote:

> On 19-02-21, 19:45, Yue Hu wrote:
> > We will set next_f to next_freq(previous freq) if next_f is
> > reduced for busy CPU. Then the next sugov_update_next_freq() will check
> > if next_freq matches next_f if need_freq_update is not set.
> > Obviously, we will do nothing for the case. And The related check to
> > fast_switch_enabled and raw_spin_{lock,unlock} operations are
> > unnecessary.  
> 
> Right, but we will still need sugov_update_next_freq() to have the
> same implementation regardless and so I am not sure if we should add

Yes, sugov_update_next_freq() should be keeping current logic for corner case.

> this change:
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 41e498b0008a..7289e1adab73 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
>          * recently, as the reduction is likely to be premature then.
>          */
>         if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> +               if (!sg_policy->need_freq_update)

The initial purpose about code of `next_f = sg_policy->next_freq` here (for special CPU busy
case) should be skipping the freq update.

Since commit 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change"),
we add the check to busy CPU for not skipping the update, we need to update the freq using
computed one because limits change.

After commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update
is set"), we removed the need_freq_update check(no issue of commit 600f5badb78c anymore?)
and introduce to always do an update in sugov_update_next_freq() if need_freq_update is set
even though current freq == sg_policy->next_freq because of corner case issue. But that is
conflict with original purpose of the freq skip code (next_f = sg_policy->next_freq) of
busy CPU. 

> +                       return;
> +

Yes, it's what i want ot add for unnecessary behaviors i mentioned above. Add return here
should just be for skipping update(different from corner case in commit23a881852f3e).

>                 next_f = sg_policy->next_freq;
>  
>                 /* Restore cached freq as next_freq has changed */
> 
>