Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1646522 > unrolled thread

[PATCH] cpufreq: dt: Set default policy->transition_delay_ns

Back to article view | Back to linux.kernel

Contents

  [PATCH] cpufreq: dt: Set default policy->transition_delay_ns Viresh Kumar <viresh.kumar@linaro.org> - 2017-05-22 07:20 +0200
    Re: [PATCH] cpufreq: dt: Set default policy->transition_delay_ns Brendan Jackman <brendan.jackman@arm.com> - 2017-05-22 12:50 +0200
      Re: [PATCH] cpufreq: dt: Set default policy->transition_delay_ns Viresh Kumar <viresh.kumar@linaro.org> - 2017-05-22 13:00 +0200
        Re: [PATCH] cpufreq: dt: Set default policy->transition_delay_ns Leo Yan <leo.yan@linaro.org> - 2017-05-22 13:20 +0200
          Re: [PATCH] cpufreq: dt: Set default policy->transition_delay_ns Viresh Kumar <viresh.kumar@linaro.org> - 2017-05-22 13:30 +0200

#1646522 — [PATCH] cpufreq: dt: Set default policy->transition_delay_ns

The rate_limit_us for the schedutil governor is getting set to 500 ms by
default for the ARM64 hikey board. And its way too much, even for the
default value. Lets set the default transition_delay_ns to something
more realistic (10 ms), while the userspace always have a chance to set
something it wants.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index c943787d761e..70eac3fd89ac 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -275,6 +275,9 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->cpuinfo.transition_latency = transition_latency;
 
+	/* Set the default transition delay to 10ms */
+	policy->transition_delay_us = 10 * USEC_PER_MSEC;
+
 	return 0;
 
 out_free_cpufreq_table:
-- 
2.7.4

[toc] | [next] | [standalone]

#1646727

Hi Viresh,

On Mon, May 22 2017 at 05:10, Viresh Kumar wrote:
> The rate_limit_us for the schedutil governor is getting set to 500 ms by
> default for the ARM64 hikey board. And its way too much, even for the
> default value. Lets set the default transition_delay_ns to something
> more realistic (10 ms), while the userspace always have a chance to set
> something it wants.

Just a thought - do you think we can treat the reported transition
latency as a proxy for the "cost" of freq transitions?  I.e. assume that
on platforms with very fast frequency switching it's probably cheap to
switch frequency and we want schedutil to respond quickly, whereas on
platforms with big latencies, frequency switches might be expensive and
we probably want hysteresis.

If that makes sense then maybe we could use 10 * transition_latency /
NSEC_PER_USEC, when transition_latency is reported? Otherwise 10ms seems
sensible to me..

Cheers,
Brendan

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq-dt.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index c943787d761e..70eac3fd89ac 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -275,6 +275,9 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>
>  	policy->cpuinfo.transition_latency = transition_latency;
>
> +	/* Set the default transition delay to 10ms */
> +	policy->transition_delay_us = 10 * USEC_PER_MSEC;
> +
>  	return 0;
>
>  out_free_cpufreq_table:

[toc] | [prev] | [next] | [standalone]

#1646729

On 22-05-17, 11:45, Brendan Jackman wrote:
> Hi Viresh,
> 
> On Mon, May 22 2017 at 05:10, Viresh Kumar wrote:
> > The rate_limit_us for the schedutil governor is getting set to 500 ms by
> > default for the ARM64 hikey board. And its way too much, even for the
> > default value. Lets set the default transition_delay_ns to something
> > more realistic (10 ms), while the userspace always have a chance to set
> > something it wants.
> 
> Just a thought - do you think we can treat the reported transition
> latency as a proxy for the "cost" of freq transitions?  I.e. assume that
> on platforms with very fast frequency switching it's probably cheap to
> switch frequency and we want schedutil to respond quickly, whereas on
> platforms with big latencies, frequency switches might be expensive and
> we probably want hysteresis.
> 
> If that makes sense then maybe we could use 10 * transition_latency /
> NSEC_PER_USEC, when transition_latency is reported? Otherwise 10ms seems
> sensible to me..

So my platform (hikey) does provide transition-latency as 500 us. But
schedutil multiplies that with LATENCY_MULTIPLIER (1000) and that
makes it 500000 rate_limit_us, which is unacceptable.

@Rafael: Why does the LATENCY_MULTIPLIER has such a high value? I am
not sure I understood completely on why we have this multiplier :(

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1646757

On Mon, May 22, 2017 at 04:25:22PM +0530, Viresh Kumar wrote:
> On 22-05-17, 11:45, Brendan Jackman wrote:
> > Hi Viresh,
> > 
> > On Mon, May 22 2017 at 05:10, Viresh Kumar wrote:
> > > The rate_limit_us for the schedutil governor is getting set to 500 ms by
> > > default for the ARM64 hikey board. And its way too much, even for the
> > > default value. Lets set the default transition_delay_ns to something
> > > more realistic (10 ms), while the userspace always have a chance to set
> > > something it wants.
> > 
> > Just a thought - do you think we can treat the reported transition
> > latency as a proxy for the "cost" of freq transitions?  I.e. assume that
> > on platforms with very fast frequency switching it's probably cheap to
> > switch frequency and we want schedutil to respond quickly, whereas on
> > platforms with big latencies, frequency switches might be expensive and
> > we probably want hysteresis.
> > 
> > If that makes sense then maybe we could use 10 * transition_latency /
> > NSEC_PER_USEC, when transition_latency is reported? Otherwise 10ms seems
> > sensible to me..
> 
> So my platform (hikey) does provide transition-latency as 500 us. But
> schedutil multiplies that with LATENCY_MULTIPLIER (1000) and that
> makes it 500000 rate_limit_us, which is unacceptable.
> 
> @Rafael: Why does the LATENCY_MULTIPLIER has such a high value? I am
> not sure I understood completely on why we have this multiplier :(

This afternoon Amit pointed me for this patch, should fix as below?
Otherwise it seems directly assign the same value from unit 'ns' to
'us' but without any value conversion.

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 76877a6..dcc90fc 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -538,7 +538,7 @@ static int sugov_init(struct cpufreq_policy *policy)
                unsigned int lat;
 
                tunables->rate_limit_us = LATENCY_MULTIPLIER;
-               lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
+               lat = policy->cpuinfo.transition_latency / NSEC_PER_MSEC;
                if (lat)
                        tunables->rate_limit_us *= lat;
        }

Thanks,
Leo Yan

[toc] | [prev] | [next] | [standalone]

#1646762

On 22-05-17, 19:17, Leo Yan wrote:
> This afternoon Amit pointed me for this patch, should fix as below?
> Otherwise it seems directly assign the same value from unit 'ns' to
> 'us' but without any value conversion.
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 76877a6..dcc90fc 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -538,7 +538,7 @@ static int sugov_init(struct cpufreq_policy *policy)
>                 unsigned int lat;
>  
>                 tunables->rate_limit_us = LATENCY_MULTIPLIER;
> -               lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
> +               lat = policy->cpuinfo.transition_latency / NSEC_PER_MSEC;
>                 if (lat)
>                         tunables->rate_limit_us *= lat;
>         }

I will let Rafael comment in as well. NSEC_PER_USEC is used in the
earlier governors as well (ondemand/conservative) in exactly the same
way as schedutil is using.

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web