linux-kernel - Re: [PATCH] cpufreq: schedutil: Don't use the limits

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: <CAJZ5v0hmgQp--uhRMZbqZnOvQPy9zLfQx_u=xAewmV=LgT6rPA@mail.gmail.com>
Date:   Fri, 12 Feb 2021 17:14:03 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Yue Hu <zbestahu@...il.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Benjamin Segall <bsegall@...gle.com>,
        Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Yue Hu <huyue2@...ong.com>, zbestahu@....com,
        zhangwen@...ong.com
Subject: Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more

On Mon, Feb 8, 2021 at 4:08 AM Yue Hu <zbestahu@...il.com> wrote:
>
> From: Yue Hu <huyue2@...ong.com>
>
> 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.

> 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.

> So, we may skip this update.

I'm not sure why?

> Hence, let's remove it to avoid above issue and make code more simple.
>
> Signed-off-by: Yue Hu <huyue2@...ong.com>
> ---
>  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.

>  }
>
>  struct cpufreq_governor schedutil_gov = {
> --
> 1.9.1
>