LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Viresh Kumar <viresh.kumar@linaro.org>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rafael Wysocki <rjw@rjwysocki.net>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Claudio Scordino <claudio@evidence.eu.com>,
	Patrick Bellasi <patrick.bellasi@arm.com>,
	Juri Lelli <juri.lelli@redhat.com>,
	Joel Fernandes <joelaf@google.com>,
	"4 . 12+" <stable@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] sched/schedutil: Don't set next_freq to UINT_MAX
Date: Wed, 9 May 2018 14:45:19 +0530	[thread overview]
Message-ID: <20180509091519.czq5zu5l7xfhqph4@vireshk-i7> (raw)
In-Reply-To: <CAJZ5v0gAuhUsEg_yLw1kZYE_MnaLFR+X1c9OZCROF2u17RaKOg@mail.gmail.com>

On 09-05-18, 10:56, Rafael J. Wysocki wrote:
> I'm kind of concerned about updating the limits via sysfs in which
> case the cached next frequency may be out of range, so it's better to
> invalidate it right away then.

That should not be a problem as __cpufreq_driver_target() will anyway
clamp the target frequency to be within limits, whatever the cached
value of next_freq is.

And we aren't invalidating the cached next freq immediately currently
as well, as we are waiting until the next time the util update handler
is called to set sg_policy->next_freq to UINT_MAX.

> > What else do you have in mind to solve this problem ?
> 
> Something like the below?
> 
> ---
>  kernel/sched/cpufreq_schedutil.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -305,7 +305,8 @@ static void sugov_update_single(struct u
>       * Do not reduce the frequency if the CPU has not been idle
>       * recently, as the reduction is likely to be premature then.
>       */
> -    if (busy && next_f < sg_policy->next_freq) {
> +    if (busy && next_f < sg_policy->next_freq &&
> +        sg_policy->next_freq != UINT_MAX) {
>          next_f = sg_policy->next_freq;
> 
>          /* Reset cached freq as next_freq has changed */

This will fix the problem we have identified currently, but adding a
special meaning to next_freq == UINT_MAX invites more hidden corner
cases like the one we just found. IMHO, using next_freq only for the
*real* frequency values makes its usage more transparent and readable.
And we already have the need_freq_update flag which we can use for
this special purpose, as is done in my patch.

-- 
viresh

  reply	other threads:[~2018-05-09  9:15 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-08  6:42 Viresh Kumar
2018-05-08 20:54 ` Rafael J. Wysocki
2018-05-09  8:41   ` Viresh Kumar
2018-05-09  8:56     ` Rafael J. Wysocki
2018-05-09  9:15       ` Viresh Kumar [this message]
2018-05-09  9:23         ` Rafael J. Wysocki
2018-05-09  9:30           ` Viresh Kumar
2018-05-09  9:32             ` Rafael J. Wysocki
2018-05-09  9:44 ` [PATCH] cpufreq: schedutil: Avoid using invalid next_freq Rafael J. Wysocki
2018-05-09  9:46   ` Viresh Kumar
2018-05-09 10:35   ` [PATCH V2] sched/schedutil: Don't set next_freq to UINT_MAX Viresh Kumar
2018-05-11 20:47     ` [V2] " Joel Fernandes
2018-05-17 10:33       ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180509091519.czq5zu5l7xfhqph4@vireshk-i7 \
    --to=viresh.kumar@linaro.org \
    --cc=claudio@evidence.eu.com \
    --cc=joelaf@google.com \
    --cc=juri.lelli@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=patrick.bellasi@arm.com \
    --cc=peterz@infradead.org \
    --cc=rafael@kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=stable@vger.kernel.org \
    --cc=vincent.guittot@linaro.org \
    --subject='Re: [PATCH] sched/schedutil: Don'\''t set next_freq to UINT_MAX' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).