On Fri, 12 Feb 2021 17:14:03 +0100
"Rafael J. Wysocki" [off-list ref] wrote:

On Mon, Feb 8, 2021 at 4:08 AM Yue Hu [off-list ref] wrote:

quoted

From: Yue Hu <redacted>

The limits_changed flag was introduced by commit 600f5badb78c
("cpufreq: schedutil: Don't skip freq update when limits change")
due to race condition where need_freq_update is cleared in
get_next_freq() which causes reducing the CPU frequency is
ineffective while busy.

But now, the race condition above is gone because get_next_freq()
doesn't clear the flag any more after commit 23a881852f3e ("cpufreq:
schedutil: Don't skip freq update if need_freq_update is set").

Moreover, need_freq_update currently will be set to true only in
sugov_should_update_freq() if CPUFREQ_NEED_UPDATE_LIMITS is not set
for the driver. However, limits may have changed at any time.  

Yes, they may change at any time.

quoted

And subsequent frequence update is depending on need_freq_update.  

I'm not following, sorry.

need_freq_update is set in sugov_should_update_freq() when
limits_changed is cleared and it cannot be modified until
sugov_update_next_freq() runs on the same CPU.

get_next_freq() will check if need to update freq not by
limits_changed but by need_freq_update.

And sugov_update_next_freq() is also checking need_freq_update to
update freq or not.

quoted

So, we may skip this update.  

I'm not sure why?

As mentioned above, need_freq_update may will not be set when
limits_changed is set since set need_freq_update happens only in
sugov_should_update_freq(), so i understand we may skip this update
or not update in time.

quoted

Hence, let's remove it to avoid above issue and make code more
simple.

Signed-off-by: Yue Hu <redacted>
---
 kernel/sched/cpufreq_schedutil.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c

b/kernel/sched/cpufreq_schedutil.c index 41e498b..7dd85fb 100644

--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c

@@ -40,7 +40,6 @@ struct sugov_policy {
        struct task_struct      *thread;
        bool                    work_in_progress;

-       bool                    limits_changed;
        bool                    need_freq_update;
 };

@@ -89,11 +88,8 @@ static bool sugov_should_update_freq(struct

sugov_policy *sg_policy, u64 time) if
(!cpufreq_this_cpu_can_update(sg_policy->policy)) return false;

-       if (unlikely(sg_policy->limits_changed)) {
-               sg_policy->limits_changed = false;
-               sg_policy->need_freq_update = true;
+       if (unlikely(sg_policy->need_freq_update))
                return true;
-       }

        delta_ns = time - sg_policy->last_freq_update_time;

@@ -323,7 +319,7 @@ static bool sugov_cpu_is_busy(struct sugov_cpu

*sg_cpu) static inline void ignore_dl_rate_limit(struct sugov_cpu
*sg_cpu, struct sugov_policy *sg_policy) {
        if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_dl)
-               sg_policy->limits_changed = true;
+               sg_policy->need_freq_update = true;
 }

 static inline bool sugov_update_single_common(struct sugov_cpu
*sg_cpu, @@ -759,7 +755,6 @@ static int sugov_start(struct
cpufreq_policy *policy) sg_policy->last_freq_update_time        = 0;
        sg_policy->next_freq                    = 0;
        sg_policy->work_in_progress             = false;
-       sg_policy->limits_changed               = false;
        sg_policy->cached_raw_freq              = 0;

        sg_policy->need_freq_update =
cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS); @@ -813,7
+808,7 @@ static void sugov_limits(struct cpufreq_policy *policy)
mutex_unlock(&sg_policy->work_lock); }

-       sg_policy->limits_changed = true;
+       sg_policy->need_freq_update = true;  

This may be running in parallel with sugov_update_next_freq() on a
different CPU, so the latter may clear need_freq_update right after it
has been set here unless I'm overlooking something.

Whether this logic is also happening for limits_changed in
sugo_should_update_freq() or not?

quoted

 }

 struct cpufreq_governor schedutil_gov = {
--
1.9.1