References: <3498238.liCqOyIkGA@aspire.rjw.lan> <2407280.n9qVSLCrF5@aspire.rjw.lan>
User-agent: mu4e 0.9.17; emacs 25.1.1
From: Brendan Jackman <brendan.jackman@arm.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Linux PM <linux-pm@vger.kernel.org>, Juri Lelli <juri.lelli@arm.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Patrick Bellasi <patrick.bellasi@arm.com>,
        Joel Fernandes <joelaf@google.com>,
        Morten Rasmussen <morten.rasmussen@arm.com>
Subject: Re: [RFC/RFT][PATCH 1/2] cpufreq: schedutil: Use policy-dependent latency multupliers
In-reply-to: <2407280.n9qVSLCrF5@aspire.rjw.lan>
Date: Mon, 10 Apr 2017 11:38:55 +0100
Message-ID: <87k26sv96o.fsf@arm.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4106
Lines: 103

Hi Rafael,

On Mon, Apr 10 2017 at 00:10, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Make the schedutil governor compute the initial (default) value of
> the rate_limit_us sysfs attribute by multiplying the transition
> latency by a multiplier depending on the policy and set by the
> scaling driver (instead of using a constant for this purpose).
>
> That will allow scaling drivers to make schedutil use smaller default
> values of rate_limit_us and reduce the default average time interval
> between consecutive frequency changes.
>

I've been thinking about this over the last couple of days, and I'm
thinking (in opposition to what I said at OSPM Pisa) that allowing
drivers to specify a _multiplier_ isn't ideal, since you lose
granularity when you want your rate limit to be close to your transition
latency (i.e. if your multiplier would be 2.5 or something).

Can we instead just have an independent field
policy->default_rate_limit_us or similar? Drivers know the transition
latency so intel_pstate can still use a multiplier if it wants.

Cheers,
Brendan

> Make intel_pstate use the opportunity to reduce the rate limit
> somewhat.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c        |    1 +
>  drivers/cpufreq/intel_pstate.c   |    2 ++
>  include/linux/cpufreq.h          |    8 ++++++++
>  kernel/sched/cpufreq_schedutil.c |    2 +-
>  4 files changed, 12 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -1072,6 +1072,7 @@ static struct cpufreq_policy *cpufreq_po
>  	init_waitqueue_head(&policy->transition_wait);
>  	init_completion(&policy->kobj_unregister);
>  	INIT_WORK(&policy->update, handle_update);
> +	policy->latency_multiplier = LATENCY_MULTIPLIER;
>
>  	policy->cpu = cpu;
>  	return policy;
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -120,6 +120,14 @@ struct cpufreq_policy {
>  	bool			fast_switch_possible;
>  	bool			fast_switch_enabled;
>
> +	/*
> +	 * Multiplier to apply to the transition latency to obtain the preferred
> +	 * average time interval between consecutive invocations of the driver
> +	 * to set the frequency for this policy.  Initialized by the core to the
> +	 * LATENCY_MULTIPLIER value.
> +	 */
> +	unsigned int		latency_multiplier;
> +
>  	 /* Cached frequency lookup from cpufreq_driver_resolve_freq. */
>  	unsigned int cached_target_freq;
>  	int cached_resolved_idx;
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -530,7 +530,7 @@ static int sugov_init(struct cpufreq_pol
>  		goto stop_kthread;
>  	}
>
> -	tunables->rate_limit_us = LATENCY_MULTIPLIER;
> +	tunables->rate_limit_us = policy->latency_multiplier;
>  	lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
>  	if (lat)
>  		tunables->rate_limit_us *= lat;
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -41,6 +41,7 @@
>  #define INTEL_PSTATE_HWP_SAMPLING_INTERVAL	(50 * NSEC_PER_MSEC)
>
>  #define INTEL_CPUFREQ_TRANSITION_LATENCY	20000
> +#define INTEL_CPUFREQ_LATENCY_MULTIPLIER	250
>
>  #ifdef CONFIG_ACPI
>  #include <acpi/processor.h>
> @@ -2237,6 +2238,7 @@ static int intel_cpufreq_cpu_init(struct
>  		return ret;
>
>  	policy->cpuinfo.transition_latency = INTEL_CPUFREQ_TRANSITION_LATENCY;
> +	policy->latency_multiplier = INTEL_CPUFREQ_LATENCY_MULTIPLIER;
>  	/* This reflects the intel_pstate_get_cpu_pstates() setting. */
>  	policy->cur = policy->cpuinfo.min_freq;
>