All of lore.kernel.org
 help / color / mirror / Atom feed
From: Leonard Crestez <leonard.crestez@nxp.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: <linux-pm@vger.kernel.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	<linux@dominikbrodowski.net>, <linux-kernel@vger.kernel.org>,
	Rafael Wysocki <rjw@rjwysocki.net>
Subject: Re: [PATCH V3 3/9] cpufreq: Cap the default transition delay value to 10 ms
Date: Thu, 27 Jul 2017 19:54:13 +0300	[thread overview]
Message-ID: <1501174453.7957.30.camel@nxp.com> (raw)
In-Reply-To: <20170726060634.GY352@vireshk-i7>

On Wed, 2017-07-26 at 11:36 +0530, Viresh Kumar wrote:
> On 25-07-17, 14:54, Leonard Crestez wrote:

> > This patch made it's way into linux-next and it seems to cause imx socs
> > to almost always hang around their max frequency with the ondemand
> > governor, even when almost completely idle. The lowest frequency is
> > never reached. This seems wrong?

> > This driver calculates transition_latency at probe time, the value is
> > not terribly accurate but it reaches values like latency = 109 us, so

> So this is the value that is stored in the global variable
> "transition_latency" in the imx6q-cpufreq.c file? i.e.
> transition_latency = 109000 (ns) to be exact ?

Yes.

> - Don't use this patch and try to change ondemand's sampling rate from
>   sysfs. Try setting it to 10000 and see if the behavior is identical
>   to after this patch.

Yes, it seems to be. Also setting 100000 explicitly fixes this.

I also tried to switch from HZ=100 to HZ=1000 but that did not make a
difference.

> - Find how much time does it really take to change the frequency of
>   the CPU. I don't really thing 109 us is the right transition
>   latency. Use attached patch for that and look for the print message.

Your patch measures latencies of around 2.5ms, but it can vary between
1.6 ms to 3ms from boot-to-boot. This is a lot more than what the
driver reports. Most transitions seem to be faster.

I did a little digging and it seems that a majority of time is always
spent inside clk_pllv3_wait_lock which spins on a HW bit while doing
usleep_range(50, 500). I originally thought it was because of
regulators but the delays involved in that are smaller.

Measuring wall time on a process that can sleep seems dubious, isn't
this vulnerable to random delays because of other tasks?

> Without this patch the sampling rate of ondemand governor will be 109
> ms. And after this patch it would be capped at 10 ms. Why would that
> screw up anyone's setup ? I don't have an answer to that right now.

On a closer look it seems that most of the time is actually spent at
low cpufreq though (90%+).

Your change makes it so that even something like "sleep 1; cat
scaling_cur_freq" raises the frequency to the maximum. This happens
enough that even if you do it in a loop you will never see the minimum
frequency. It seems there is enough internal bookkeeping on such a
wakeup that it takes more than 10ms and enough for a reevaluation of
cpufreq until cat returns the value?!
 
I found this by enabling the power:cpu_frequency tracepoint event and
checking for deltas with a script. Enabling CPU_FREQ_STAT show this:

time_in_state:

396000 1609
792000 71
996000 54

trans_table:

   From  :    To
         :    396000    792000    996000 
   396000:         0        10         7 
   792000:        16         0        12 
   996000:         1        18         0 

This is very unexpected but not necessarily wrong.

--
Regards,
Leonard

WARNING: multiple messages have this Message-ID (diff)
From: Leonard Crestez <leonard.crestez@nxp.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	linux@dominikbrodowski.net, linux-kernel@vger.kernel.org,
	Rafael Wysocki <rjw@rjwysocki.net>
Subject: Re: [PATCH V3 3/9] cpufreq: Cap the default transition delay value to 10 ms
Date: Thu, 27 Jul 2017 19:54:13 +0300	[thread overview]
Message-ID: <1501174453.7957.30.camel@nxp.com> (raw)
In-Reply-To: <20170726060634.GY352@vireshk-i7>

On Wed, 2017-07-26 at 11:36 +0530, Viresh Kumar wrote:
> On 25-07-17, 14:54, Leonard Crestez wrote:

> > This patch made it's way into linux-next and it seems to cause imx socs
> > to almost always hang around their max frequency with the ondemand
> > governor, even when almost completely idle. The lowest frequency is
> > never reached. This seems wrong?

> > This driver calculates transition_latency at probe time, the value is
> > not terribly accurate but it reaches values like latency = 109 us, so

> So this is the value that is stored in the global variable
> "transition_latency" in the imx6q-cpufreq.c file? i.e.
> transition_latency = 109000 (ns) to be exact ?

Yes.

> - Don't use this patch and try to change ondemand's sampling rate from
>   sysfs. Try setting it to 10000 and see if the behavior is identical
>   to after this patch.

Yes, it seems to be. Also setting 100000 explicitly fixes this.

I also tried to switch from HZ=100 to HZ=1000 but that did not make a
difference.

> - Find how much time does it really take to change the frequency of
>   the CPU. I don't really thing 109 us is the right transition
>   latency. Use attached patch for that and look for the print message.

Your patch measures latencies of around 2.5ms, but it can vary between
1.6 ms to 3ms from boot-to-boot. This is a lot more than what the
driver reports. Most transitions seem to be faster.

I did a little digging and it seems that a majority of time is always
spent inside clk_pllv3_wait_lock which spins on a HW bit while doing
usleep_range(50, 500). I originally thought it was because of
regulators but the delays involved in that are smaller.

Measuring wall time on a process that can sleep seems dubious, isn't
this vulnerable to random delays because of other tasks?

> Without this patch the sampling rate of ondemand governor will be 109
> ms. And after this patch it would be capped at 10 ms. Why would that
> screw up anyone's setup ? I don't have an answer to that right now.

On a closer look it seems that most of the time is actually spent at
low cpufreq though (90%+).

Your change makes it so that even something like "sleep 1; cat
scaling_cur_freq" raises the frequency to the maximum. This happens
enough that even if you do it in a loop you will never see the minimum
frequency. It seems there is enough internal bookkeeping on such a
wakeup that it takes more than 10ms and enough for a reevaluation of
cpufreq until cat returns the value?!
 
I found this by enabling the power:cpu_frequency tracepoint event and
checking for deltas with a script. Enabling CPU_FREQ_STAT show this:

time_in_state:

396000 1609
792000 71
996000 54

trans_table:

   From  :    To
         :    396000    792000    996000 
   396000:         0        10         7 
   792000:        16         0        12 
   996000:         1        18         0 

This is very unexpected but not necessarily wrong.

--
Regards,
Leonard

  reply	other threads:[~2017-07-27 16:54 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-19 10:12 [PATCH V3 0/9] cpufreq: transition-latency cleanups Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 1/9] cpufreq: governor: Drop min_sampling_rate Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 2/9] cpufreq: Use transition_delay_us for legacy governors as well Viresh Kumar
2017-07-24 16:17   ` Peter Zijlstra
2017-07-28  4:48     ` Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 3/9] cpufreq: Cap the default transition delay value to 10 ms Viresh Kumar
2017-07-25 11:54   ` Leonard Crestez
2017-07-25 11:54     ` Leonard Crestez
2017-07-26  0:19     ` Rafael J. Wysocki
2017-07-26  6:06     ` Viresh Kumar
2017-07-27 16:54       ` Leonard Crestez [this message]
2017-07-27 16:54         ` Leonard Crestez
2017-07-28  5:28         ` Viresh Kumar
2017-08-01 17:48           ` Leonard Crestez
2017-08-01 17:48             ` Leonard Crestez
2017-08-02  3:23             ` Viresh Kumar
2017-08-16  6:34           ` Viresh Kumar
2017-08-16  9:42             ` Leonard Crestez
2017-08-16  9:42               ` Leonard Crestez
2017-08-17  3:38               ` Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 4/9] cpufreq: Don't set transition_latency for setpolicy drivers Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 5/9] cpufreq: arm_big_little: Make ->get_transition_latency() mandatory Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 6/9] cpufreq: Replace "max_transition_latency" with "dynamic_switching" Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 7/9] cpufreq: schedutil: Set dynamic_switching to true Viresh Kumar
2017-07-19 10:12 ` [PATCH V3 8/9] cpufreq: Add CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING cpufreq driver flag Viresh Kumar
2017-07-19 16:30   ` Dominik Brodowski
2017-07-19 10:12 ` [PATCH V3 9/9] cpufreq: Allow dynamic switching with CPUFREQ_ETERNAL latency Viresh Kumar
2017-07-19 12:42 ` [PATCH V3 0/9] cpufreq: transition-latency cleanups 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=1501174453.7957.30.camel@nxp.com \
    --to=leonard.crestez@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux@dominikbrodowski.net \
    --cc=rjw@rjwysocki.net \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.