Re: [RFC PATCH] cpufreq: powernv: Add fast_switch callback

Rafael J. Wysocki
2016-05-18T21:22:22Z

On Wed, May 18, 2016 at 2:53 PM, Shilpasri G Bhat
<shilpa.bhat@linux.vnet.ibm.com> wrote:
> Add fast_switch driver callback to support frequency update in
> interrupt context while using schedutil governor. Changing frequency
> in interrupt context will remove the jitter on the workloads which can
> be seen when a kworker thread is used for the changing the frequency.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>

This looks simple enough. :-)

A couple of comments, though.

> ---
> drivers/cpufreq/powernv-cpufreq.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 54c4536..4553eb6 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -678,6 +678,8 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> for (i = 0; i < threads_per_core; i++)
> cpumask_set_cpu(base + i, policy->cpus);
>
> + policy->fast_switch_possible = true;
> +
> kn = kernfs_find_and_get(policy->kobj.sd, throttle_attr_grp.name);
> if (!kn) {
> int ret;
> @@ -854,6 +856,24 @@ static void powernv_cpufreq_stop_cpu(struct cpufreq_policy *policy)
> del_timer_sync(&gpstates->timer);
> }
>
> +static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + int index;
> + struct powernv_smp_call_data freq_data;
> +
> + cpufreq_frequency_table_target(policy, policy->freq_table,
> + target_freq,
> + CPUFREQ_RELATION_C, &index);

According to the discussion I had with Peter some time ago, this
should be RELATION_L or you may end up using a frequency that's not
sufficient to meet a deadline somewhere.

Also cpufreq_frequency_table_target() is somewhat heavy-weight
especially if the table is known to be sorted (which I guess is the
case).

> + if (index < 0 || index >= powernv_pstate_info.nr_pstates)
> + return CPUFREQ_ENTRY_INVALID;
> + freq_data.pstate_id = powernv_freqs[index].driver_data;
> + freq_data.gpstate_id = powernv_freqs[index].driver_data;
> + set_pstate(&freq_data);
> +
> + return pstate_id_to_freq(-index);
> +}
> +
> static struct cpufreq_driver powernv_cpufreq_driver = {
> .name = "powernv-cpufreq",
> .flags = CPUFREQ_CONST_LOOPS,
> @@ -861,6 +881,7 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
> .exit = powernv_cpufreq_cpu_exit,
> .verify = cpufreq_generic_frequency_table_verify,
> .target_index = powernv_cpufreq_target_index,
> + .fast_switch = powernv_fast_switch,
> .get = powernv_cpufreq_get,
> .stop_cpu = powernv_cpufreq_stop_cpu,
> .attr = powernv_cpu_freq_attr,
> --