[PATCH] cpufreq: intel_pstate: Synchronize sysfs limits

[PATCH] cpufreq: intel_pstate: Synchronize sysfs limits

[Srinivas Pandruvada]

When user sets some limits using Intel P-State sysfs, they are not
reflected in the cpufreq policy scaling_max_freq and scaling_min_freq.
This change updates the cpufreq policy of each CPU, when user sets
limits via Intel P-State sysfs.

For example:
root@stn1]# cat /sys/devices/system/cpu/intel_pstate/max_perf_pct
100
root@stn1]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
2800000

Now limit the max performance
root@stn1]# echo 80 > /sys/devices/system/cpu/intel_pstate/max_perf_pct
This change now is also changed the scaling_max_freq
root@stn1]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
2240000

But there is a side effect of this change for the users who use both
methods to limit interchangeably. For example if user sets limit via
Intel P-State sysfs to a lower maximum value and then sets a higher
maximum value via cpufreq sysfs, then the cpufreq display will show a
lower maximum value set by the Intel P-State sysfs. But there is no
change in the effective P-State used, as the driver will always use
the most constrained limit.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/intel_pstate.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 8f683e2..d8315e3 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -805,6 +805,18 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
 	return count;
 }
 
+static void update_cpufreq_policies(void)
+{
+	int cpu;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		if (all_cpu_data[cpu])
+			cpufreq_update_policy(cpu);
+	}
+	put_online_cpus();
+}
+
 static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
 				  const char *buf, size_t count)
 {
@@ -830,6 +842,9 @@ static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
 
 	if (hwp_active)
 		intel_pstate_hwp_set_online_cpus();
+
+	update_cpufreq_policies();
+
 	return count;
 }
 
@@ -858,6 +873,9 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
 
 	if (hwp_active)
 		intel_pstate_hwp_set_online_cpus();
+
+	update_cpufreq_policies();
+
 	return count;
 }
 
@@ -1802,6 +1820,28 @@ static struct cpufreq_driver intel_pstate_driver = {
 	.name		= "intel_pstate",
 };
 
+static int cpufreq_intel_pstate_notifier(struct notifier_block *nb,
+					 unsigned long event, void *data)
+{
+	struct cpufreq_policy *policy = data;
+	int max_freq, min_freq;
+
+	/* When per-CPU limits are used, sysfs limits can't be set */
+	if (per_cpu_limits)
+		return NOTIFY_OK;
+
+	max_freq = policy->cpuinfo.max_freq * limits->max_sysfs_pct / 100;
+	min_freq = policy->cpuinfo.max_freq * limits->min_sysfs_pct / 100;
+
+	cpufreq_verify_within_limits(policy, min_freq, max_freq);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block intel_pstate_cpufreq_notifier_block = {
+	.notifier_call = cpufreq_intel_pstate_notifier,
+};
+
 static int no_load __initdata;
 static int no_hwp __initdata;
 static int hwp_only __initdata;
@@ -2032,6 +2072,9 @@ static int __init intel_pstate_init(void)
 	if (hwp_active)
 		pr_info("HWP enabled\n");
 
+	cpufreq_register_notifier(&intel_pstate_cpufreq_notifier_block,
+				  CPUFREQ_POLICY_NOTIFIER);
+
 	return rc;
 out:
 	get_online_cpus();
-- 
2.7.4

Re: [PATCH] cpufreq: intel_pstate: Synchronize sysfs limits

[Rafael J. Wysocki]

On Sat, Nov 12, 2016 at 1:11 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> When user sets some limits using Intel P-State sysfs, they are not
> reflected in the cpufreq policy scaling_max_freq and scaling_min_freq.
> This change updates the cpufreq policy of each CPU, when user sets
> limits via Intel P-State sysfs.
>
> For example:
> root@stn1]# cat /sys/devices/system/cpu/intel_pstate/max_perf_pct
> 100
> root@stn1]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 2800000
>
> Now limit the max performance
> root@stn1]# echo 80 > /sys/devices/system/cpu/intel_pstate/max_perf_pct
> This change now is also changed the scaling_max_freq
> root@stn1]# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 2240000
>
> But there is a side effect of this change for the users who use both
> methods to limit interchangeably. For example if user sets limit via
> Intel P-State sysfs to a lower maximum value and then sets a higher
> maximum value via cpufreq sysfs, then the cpufreq display will show a
> lower maximum value set by the Intel P-State sysfs. But there is no
> change in the effective P-State used, as the driver will always use
> the most constrained limit.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 43 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 8f683e2..d8315e3 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -805,6 +805,18 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
>         return count;
>  }
>
> +static void update_cpufreq_policies(void)
> +{
> +       int cpu;
> +
> +       get_online_cpus();
> +       for_each_online_cpu(cpu) {
> +               if (all_cpu_data[cpu])
> +                       cpufreq_update_policy(cpu);

cpufreq_update_policy() calls cpufreq_cpu_get() to get the policy
anyway which does the requisite policy existence check (although it is
a bit racy now, but that's a bug in there that we should not have to
work around here), so it should be sufficient to do this
for_each_possible_cpu() without additional locking.

> +       }
> +       put_online_cpus();
> +}
> +
>  static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
>                                   const char *buf, size_t count)
>  {
> @@ -830,6 +842,9 @@ static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
>
>         if (hwp_active)
>                 intel_pstate_hwp_set_online_cpus();
> +
> +       update_cpufreq_policies();
> +
>         return count;
>  }
>
> @@ -858,6 +873,9 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
>
>         if (hwp_active)
>                 intel_pstate_hwp_set_online_cpus();
> +
> +       update_cpufreq_policies();
> +
>         return count;
>  }
>
> @@ -1802,6 +1820,28 @@ static struct cpufreq_driver intel_pstate_driver = {
>         .name           = "intel_pstate",
>  };
>
> +static int cpufreq_intel_pstate_notifier(struct notifier_block *nb,
> +                                        unsigned long event, void *data)
> +{
> +       struct cpufreq_policy *policy = data;
> +       int max_freq, min_freq;
> +
> +       /* When per-CPU limits are used, sysfs limits can't be set */
> +       if (per_cpu_limits)
> +               return NOTIFY_OK;
> +
> +       max_freq = policy->cpuinfo.max_freq * limits->max_sysfs_pct / 100;
> +       min_freq = policy->cpuinfo.max_freq * limits->min_sysfs_pct / 100;
> +
> +       cpufreq_verify_within_limits(policy, min_freq, max_freq);
> +
> +       return NOTIFY_OK;
> +}
> +
> +static struct notifier_block intel_pstate_cpufreq_notifier_block = {
> +       .notifier_call = cpufreq_intel_pstate_notifier,
> +};
> +
>  static int no_load __initdata;
>  static int no_hwp __initdata;
>  static int hwp_only __initdata;
> @@ -2032,6 +2072,9 @@ static int __init intel_pstate_init(void)
>         if (hwp_active)
>                 pr_info("HWP enabled\n");
>
> +       cpufreq_register_notifier(&intel_pstate_cpufreq_notifier_block,
> +                                 CPUFREQ_POLICY_NOTIFIER);
> +

cpufreq_set_policy() will call our ->verify() and ->set_policy()
things, so why do we need the notifier?

>         return rc;
>  out:
>         get_online_cpus();
> --

Thanks,
Rafael

Re: [PATCH] cpufreq: intel_pstate: Synchronize sysfs limits

[Srinivas Pandruvada]

On Mon, 2016-11-14 at 02:04 +0100, Rafael J. Wysocki wrote:
> On Sat, Nov 12, 2016 at 1:11 AM, Srinivas Pandruvada
> 
[...]
> +       get_online_cpus();
> > +       for_each_online_cpu(cpu) {
> > +               if (all_cpu_data[cpu])
> > +                       cpufreq_update_policy(cpu);
> 
> cpufreq_update_policy() calls cpufreq_cpu_get() to get the policy
> anyway which does the requisite policy existence check (although it
> is
> a bit racy now, but that's a bug in there that we should not have to
> work around here), so it should be sufficient to do this
> for_each_possible_cpu() without additional locking.
> 
I will change in the next patch set.

> > 
[...]

> > +       cpufreq_register_notifier(&intel_pstate_cpufreq_notifier_bl
> > ock,
> > +                                 CPUFREQ_POLICY_NOTIFIER);
> > +
> 
> cpufreq_set_policy() will call our ->verify() and ->set_policy()
> things, so why do we need the notifier?
> 
I was simply replicating what is done for _PPC notifiers. But we can do
in verify() callback here. In the next patch, I will change this.

Thanks,
Srinivas

Re: [PATCH] cpufreq: intel_pstate: Synchronize sysfs limits

[Rafael J. Wysocki]

On Mon, Nov 14, 2016 at 11:07 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Mon, 2016-11-14 at 02:04 +0100, Rafael J. Wysocki wrote:
>> On Sat, Nov 12, 2016 at 1:11 AM, Srinivas Pandruvada
>>
> [...]
>> +       get_online_cpus();
>> > +       for_each_online_cpu(cpu) {
>> > +               if (all_cpu_data[cpu])
>> > +                       cpufreq_update_policy(cpu);
>>
>> cpufreq_update_policy() calls cpufreq_cpu_get() to get the policy
>> anyway which does the requisite policy existence check (although it
>> is
>> a bit racy now, but that's a bug in there that we should not have to
>> work around here), so it should be sufficient to do this
>> for_each_possible_cpu() without additional locking.
>>
> I will change in the next patch set.

Wait, wait, there's a better way I think, but I'll need to send a
patch to explain what I mean. :-)  Will do that shortly.

Thanks,
Rafael

Re: [PATCH] cpufreq: intel_pstate: Synchronize sysfs limits

[Rafael J. Wysocki]

On Monday, November 14, 2016 11:11:39 PM Rafael J. Wysocki wrote:
> On Mon, Nov 14, 2016 at 11:07 PM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > On Mon, 2016-11-14 at 02:04 +0100, Rafael J. Wysocki wrote:
> >> On Sat, Nov 12, 2016 at 1:11 AM, Srinivas Pandruvada
> >>
> > [...]
> >> +       get_online_cpus();
> >> > +       for_each_online_cpu(cpu) {
> >> > +               if (all_cpu_data[cpu])
> >> > +                       cpufreq_update_policy(cpu);
> >>
> >> cpufreq_update_policy() calls cpufreq_cpu_get() to get the policy
> >> anyway which does the requisite policy existence check (although it
> >> is
> >> a bit racy now, but that's a bug in there that we should not have to
> >> work around here), so it should be sufficient to do this
> >> for_each_possible_cpu() without additional locking.
> >>
> > I will change in the next patch set.
> 
> Wait, wait, there's a better way I think, but I'll need to send a
> patch to explain what I mean. :-)  Will do that shortly.

Something like the below, modulo changes in ->verify() or in ->set_policy() to
update min/max (untested, but I don't see why it wouldn't work).

If we are going to call cpufreq_update_policy() anyway, we don't need to do
anything special for HWP then, because that will trigger ->set_policy()
eventually and we'll update HWP at the end of it.

---
 drivers/cpufreq/intel_pstate.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -572,13 +572,13 @@ static inline void update_turbo_state(vo
 		 cpu->pstate.max_pstate == cpu->pstate.turbo_pstate);
 }
 
-static void intel_pstate_hwp_set(const struct cpumask *cpumask)
+static void intel_pstate_hwp_set(struct cpufreq_policy *policy)
 {
 	int min, hw_min, max, hw_max, cpu, range, adj_range;
 	struct perf_limits *perf_limits = limits;
 	u64 value, cap;
 
-	for_each_cpu(cpu, cpumask) {
+	for_each_cpu(cpu, policy->cpus) {
 		int max_perf_pct, min_perf_pct;
 
 		if (per_cpu_limits)
@@ -615,16 +615,17 @@ static void intel_pstate_hwp_set(const s
 static int intel_pstate_hwp_set_policy(struct cpufreq_policy *policy)
 {
 	if (hwp_active)
-		intel_pstate_hwp_set(policy->cpus);
+		intel_pstate_hwp_set(policy);
 
 	return 0;
 }
 
-static void intel_pstate_hwp_set_online_cpus(void)
+static void intel_pstate_update_policies(void)
 {
-	get_online_cpus();
-	intel_pstate_hwp_set(cpu_online_mask);
-	put_online_cpus();
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		cpufreq_update_policy(cpu);
 }
 
 /************************** debugfs begin ************************/
@@ -751,9 +752,7 @@ static ssize_t store_no_turbo(struct kob
 
 	mutex_unlock(&intel_pstate_limits_lock);
 
-	if (hwp_active)
-		intel_pstate_hwp_set_online_cpus();
-
+	intel_pstate_update_policies();
 	return count;
 }
 
@@ -780,8 +779,7 @@ static ssize_t store_max_perf_pct(struct
 
 	mutex_unlock(&intel_pstate_limits_lock);
 
-	if (hwp_active)
-		intel_pstate_hwp_set_online_cpus();
+	intel_pstate_update_policies();
 	return count;
 }
 
@@ -808,8 +806,7 @@ static ssize_t store_min_perf_pct(struct
 
 	mutex_unlock(&intel_pstate_limits_lock);
 
-	if (hwp_active)
-		intel_pstate_hwp_set_online_cpus();
+	intel_pstate_update_policies();
 	return count;
 }