On Tuesday, April 19, 2016 07:39:28 PM Steve Muckle wrote:
> In preparation for the scheduler cpufreq callback happening on remote
> CPUs, add support for this in intel_pstate, which requires the
> callback run on the local CPU to be able to change the CPU frequency.
>
> Signed-off-by: Steve Muckle <smuc...@linaro.org>
> ---
> drivers/cpufreq/intel_pstate.c | 88
> +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 6c7cff13f0ed..fa49d3944aa5 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -162,6 +162,9 @@ struct _pid {
> * struct cpudata - Per CPU instance data storage
> * @cpu: CPU number for this instance data
> * @update_util: CPUFreq utility callback information
> + * @irq_work: Data for passing remote callbacks to the target
> CPU
> + * @time: Timestamp of CPUFreq callback
> + * @ipi_in_progress: Whether a remote callback IPI is outstanding
> * @pstate: Stores P state limits for this CPU
> * @vid: Stores VID limits for this CPU
> * @pid: Stores PID parameters for this CPU
> @@ -179,6 +182,9 @@ struct cpudata {
> int cpu;
>
> struct update_util_data update_util;
> + struct irq_work irq_work;
> + u64 time;
> + bool ipi_in_progress;
>
> struct pstate_data pstate;
> struct vid_data vid;
> @@ -1173,20 +1179,88 @@ static inline void
> intel_pstate_adjust_busy_pstate(struct cpudata *cpu)
> get_avg_frequency(cpu));
> }
>
> +static void _intel_pstate_update_util(struct cpudata *cpu, u64 time)
What about calling this intel_pstate_update_cpu()?
> +{
> + bool sample_taken = intel_pstate_sample(cpu, time);
> +
> + if (sample_taken && !hwp_active)
> + intel_pstate_adjust_busy_pstate(cpu);
> +}
> +
> +#ifdef CONFIG_SMP
> +static void intel_pstate_update_util_remote(struct irq_work *irq_work)
> +{
> + struct cpudata *cpu = container_of(irq_work, struct cpudata, irq_work);
> + s64 delta_ns = cpu->time - cpu->sample.time;
> +
> + /*
> + * A local update may have happened while the ipi
> + * was in progress so re-check the time.
> + */
> + if (delta_ns < pid_params.sample_rate_ns)
> + return;
> +
> + _intel_pstate_update_util(cpu, cpu->time);
> +
> + cpu->ipi_in_progress = false;
> +}
> +
> static void intel_pstate_update_util(struct update_util_data *data, u64 time,
> unsigned long util, unsigned long max)
> {
> struct cpudata *cpu = container_of(data, struct cpudata, update_util);
> - u64 delta_ns = time - cpu->sample.time;
> + s64 delta_ns = time - cpu->sample.time;
>
> - if ((s64)delta_ns >= pid_params.sample_rate_ns) {
> - bool sample_taken = intel_pstate_sample(cpu, time);
> + if (delta_ns < pid_params.sample_rate_ns)
Why don't you check cpu->ipi_in_progress here too and bail out if it is set?
That would allow you to avoid checking the time again below, woulnd't it?
> + return;
>
> - if (sample_taken && !hwp_active)
> - intel_pstate_adjust_busy_pstate(cpu);
> + if (cpu->cpu == smp_processor_id()) {
> + _intel_pstate_update_util(cpu, time);
> + } else {
> + /* The target CPU's rq lock is held. */
> + if (cpu->ipi_in_progress)
> + return;
> +
> + /* Re-check sample_time which may have advanced. */
> + smp_rmb();
> + delta_ns = time - READ_ONCE(cpu->sample.time);
> + if (delta_ns < pid_params.sample_rate_ns)
> + return;
> +
> + cpu->ipi_in_progress = true;
> + cpu->time = time;
> + irq_work_queue_on(&cpu->irq_work, cpu->cpu);
> }
> }
>
> +static inline void intel_pstate_irq_work_sync(unsigned int cpu)
> +{
> + irq_work_sync(&all_cpu_data[cpu]->irq_work);
> +}
> +
> +static inline void intel_pstate_init_irq_work(struct cpudata *cpu)
> +{
> + init_irq_work(&cpu->irq_work, intel_pstate_update_util_remote);
> +}
> +#else /* !CONFIG_SMP */
> +static inline void intel_pstate_irq_work_sync(unsigned int cpu) {}
> +static inline void intel_pstate_init_irq_work(struct cpudata *cpu) {}
> +
> +static void intel_pstate_update_util(struct update_util_data *data, u64 time,
> + unsigned long util, unsigned long max)
> +{
> + struct cpudata *cpu = container_of(data, struct cpudata, update_util);
> + s64 delta_ns = time - cpu->sample.time;
> +
> + if (delta_ns < pid_params.sample_rate_ns)
> + return;
> +
> + _intel_pstate_update_util(cpu, time);
> +}
> +#endif
> +
> +
> +
The additional two empty lines are not necessary.
> #define ICPU(model, policy) \
> { X86_VENDOR_INTEL, 6, model, X86_FEATURE_APERFMPERF,\
> (unsigned long)&policy }
> @@ -1273,6 +1347,7 @@ static void
> intel_pstate_clear_update_util_hook(unsigned int cpu)
> {
> cpufreq_remove_update_util_hook(cpu);
> synchronize_sched();
> + intel_pstate_irq_work_sync(cpu);
> }
>
> static void intel_pstate_set_performance_limits(struct perf_limits *limits)
> @@ -1379,6 +1454,9 @@ static int intel_pstate_cpu_init(struct cpufreq_policy
> *policy)
>
> cpu = all_cpu_data[policy->cpu];
>
> + intel_pstate_init_irq_work(cpu);
> +
> +
One additional empty line should be sufficient here.
> if (limits->min_perf_pct == 100 && limits->max_perf_pct == 100)
> policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> else
>
Thanks,
Rafael
