cpufreq: cppc: Add support for frequency invariance

It's a classic example of memleak, we allocate something, we fail and never free the resources.

Make sure we free all resources on policy ->init() failures.

Fixes a28b2bfc099c ("cppc_cpufreq: replace per-cpu data array with a list")

Tested-by Vincent Guittot <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cppc_cpufreq.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

drivers/cpufreq/cppc_cpufreq.c +12 -1

index be4f62e2c5f1..35b8ae66d1fb 100644
@@ -256,6 +256,16 @@ static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
 	return NULL;
 }
 
+static void cppc_cpufreq_put_cpu_data(struct cpufreq_policy *policy)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+
+	list_del(&cpu_data->node);
+	free_cpumask_var(cpu_data->shared_cpu_map);
+	kfree(cpu_data);
+	policy->driver_data = NULL;
+}
+
 static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
 	unsigned int cpu = policy->cpu;

@@ -309,7 +319,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	default:
 		pr_debug("Unsupported CPU co-ord type: %d\n",
 			 policy->shared_type);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto out;
 	}
 
 	/*

@@ -324,10 +335,14 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	cpu_data->perf_ctrls.desired_perf =  caps->highest_perf;
 

drivers/cpufreq/cppc_cpufreq.c +3 -6

+out:
+	cppc_cpufreq_put_cpu_data(policy);
 	return ret;
 }
 

@@ -345,12 +360,7 @@ static int cppc_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 			 caps->lowest_perf, cpu, ret);
 
-	/* Remove CPU node from list and free driver data for policy */
-	free_cpumask_var(cpu_data->shared_cpu_map);
-	list_del(&cpu_data->node);
-	kfree(policy->driver_data);
-	policy->driver_data = NULL;
-
+	cppc_cpufreq_put_cpu_data(policy);
 	return 0;
 }
 

Signature · 1 line

2.31.1.272.g89b43f80a514

Might be better for this to be separate from the series, but I suppose all will be going in 5.14 anyway.

Nit: I would have preferred the more traditional:

if (ret) {
	pr_debug();
	goto out;
}

return 0;

It's always easier to read.

It's a classic example of memleak, we allocate something, we fail and never free the resources.

Make sure we free all resources on policy ->init() failures.

Fixes a28b2bfc099c ("cppc_cpufreq: replace per-cpu data array with a list")

Tested-by Vincent Guittot <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
V3->V3.1:
- Updated "if (!ret)" to "if (ret)", the more commonly used format.

 drivers/cpufreq/cppc_cpufreq.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

drivers/cpufreq/cppc_cpufreq.c +20 -8

index be4f62e2c5f1..945ab4942c1c 100644
@@ -256,6 +256,16 @@ static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
 	return NULL;
 }
 
+static void cppc_cpufreq_put_cpu_data(struct cpufreq_policy *policy)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+
+	list_del(&cpu_data->node);
+	free_cpumask_var(cpu_data->shared_cpu_map);
+	kfree(cpu_data);
+	policy->driver_data = NULL;
+}
+
 static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
 	unsigned int cpu = policy->cpu;

@@ -309,7 +319,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	default:
 		pr_debug("Unsupported CPU co-ord type: %d\n",
 			 policy->shared_type);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto out;
 	}
 
 	/*

@@ -324,10 +335,16 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	cpu_data->perf_ctrls.desired_perf =  caps->highest_perf;
 
 	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
-	if (ret)
+	if (ret) {
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 			 caps->highest_perf, cpu, ret);
+		goto out;
+	}
+
+	return 0;
 
+out:
+	cppc_cpufreq_put_cpu_data(policy);
 	return ret;
 }
 

@@ -345,12 +362,7 @@ static int cppc_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 			 caps->lowest_perf, cpu, ret);
 
-	/* Remove CPU node from list and free driver data for policy */
-	free_cpumask_var(cpu_data->shared_cpu_map);
-	list_del(&cpu_data->node);
-	kfree(policy->driver_data);
-	policy->driver_data = NULL;
-
+	cppc_cpufreq_put_cpu_data(policy);
 	return 0;
 }
 

Signature · 1 line

2.31.1.272.g89b43f80a514

Don't pass structure instance by value, pass it by reference instead.

Tested-by Vincent Guittot <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cppc_cpufreq.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/cpufreq/cppc_cpufreq.c +8 -8

index 35b8ae66d1fb..490175d65082 100644
@@ -373,18 +373,18 @@ static inline u64 get_delta(u64 t1, u64 t0)
 }
 
 static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data,
-				     struct cppc_perf_fb_ctrs fb_ctrs_t0,
-				     struct cppc_perf_fb_ctrs fb_ctrs_t1)
+				     struct cppc_perf_fb_ctrs *fb_ctrs_t0,
+				     struct cppc_perf_fb_ctrs *fb_ctrs_t1)
 {
 	u64 delta_reference, delta_delivered;
 	u64 reference_perf, delivered_perf;
 
-	reference_perf = fb_ctrs_t0.reference_perf;
+	reference_perf = fb_ctrs_t0->reference_perf;
 
-	delta_reference = get_delta(fb_ctrs_t1.reference,
-				    fb_ctrs_t0.reference);
-	delta_delivered = get_delta(fb_ctrs_t1.delivered,
-				    fb_ctrs_t0.delivered);
+	delta_reference = get_delta(fb_ctrs_t1->reference,
+				    fb_ctrs_t0->reference);
+	delta_delivered = get_delta(fb_ctrs_t1->delivered,
+				    fb_ctrs_t0->delivered);
 
 	/* Check to avoid divide-by zero */
 	if (delta_reference || delta_delivered)

@@ -415,7 +415,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 	if (ret)
 		return ret;
 
-	return cppc_get_rate_from_fbctrs(cpu_data, fb_ctrs_t0, fb_ctrs_t1);
+	return cppc_get_rate_from_fbctrs(cpu_data, &fb_ctrs_t0, &fb_ctrs_t1);
 }
 
 static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)

Signature · 1 line

2.31.1.272.g89b43f80a514

Quoted 2 lines · Viresh Kumar

Don't pass structure instance by value, pass it by reference instead.

Might be best to justify the change a bit :)

For me this is a judgement call, and I don't really see the reasons for changing it: we don't care if the structure is modified or not, as we're not reusing the data after the call to cppc_get_rate_from_fbctrs(). More so, in this scenario we might not even want for the called function to modify the counter values. Also there is no further call to a function in cppc_get_rate_from_fbctrs(), that might require references to the fb_ctrs.

So what is the reason behind this change?

Quoted 5 lines · Ionela Voinescu

Quoted 2 lines · Viresh Kumar

Don't pass structure instance by value, pass it by reference instead.

Might be best to justify the change a bit :)

I had it and removed later as I thought it would be obvious :)

Quoted 9 lines

For me this is a judgement call, and I don't really see the reasons for changing it: we don't care if the structure is modified or not, as we're not reusing the data after the call to cppc_get_rate_from_fbctrs(). More so, in this scenario we might not even want for the called function to modify the counter values. Also there is no further call to a function in cppc_get_rate_from_fbctrs(), that might require references to the fb_ctrs.

So what is the reason behind this change?

How about this commit log then:

Theoretically speaking, call by reference is cheaper/faster than call by value for structures as the later requires the compiler to make a new copy of the whole structure (which has four u64 values here), to be used by the called function, while with call by reference we just need to pass a single pointer (u64 on 64-bit architectures) to the existing structure.

Yes, on modern architectures, the compilers will likely end up using the processor registers for passing this structure as it isn't doesn't have lot of fields and it shouldn't be bad eventually, but nevertheless the code should do the right thing without assuming about the compiler's or architecture's optimizations.

Don't pass structure instance by value, pass it by reference instead.

Signature · 1 line

viresh

Currently topology_scale_freq_tick() (which gets called from scheduler_tick()) may end up using a pointer to "struct scale_freq_data", which was previously cleared by topology_clear_scale_freq_source(), as there is no protection in place here. The users of topology_clear_scale_freq_source() though needs a guarantee that the previously cleared scale_freq_data isn't used anymore, so they can free the related resources.

Since topology_scale_freq_tick() is called from scheduler tick, we don't want to add locking in there. Use the RCU update mechanism instead (which is already used by the scheduler's utilization update path) to guarantee race free updates here.

synchronize_rcu() makes sure that all RCU critical sections that started before it is called, will finish before it returns. And so the callers of topology_clear_scale_freq_source() don't need to worry about their callback getting called anymore.

Cc Paul E. McKenney <[email protected]>

Fixes 01e055c120a4 ("arch_topology: Allow multiple entities to provide sched_freq_tick() callback")

Tested-by Vincent Guittot <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/base/arch_topology.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

drivers/base/arch_topology.c +21 -6

index c1179edc0f3b..921312a8d957 100644
@@ -18,10 +18,11 @@
 #include <linux/cpumask.h>
 #include <linux/init.h>
 #include <linux/percpu.h>
+#include <linux/rcupdate.h>
 #include <linux/sched.h>
 #include <linux/smp.h>
 
-static DEFINE_PER_CPU(struct scale_freq_data *, sft_data);
+static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data);
 static struct cpumask scale_freq_counters_mask;
 static bool scale_freq_invariant;
 

@@ -66,16 +67,20 @@ void topology_set_scale_freq_source(struct scale_freq_data *data,
 	if (cpumask_empty(&scale_freq_counters_mask))
 		scale_freq_invariant = topology_scale_freq_invariant();
 
+	rcu_read_lock();
+
 	for_each_cpu(cpu, cpus) {
-		sfd = per_cpu(sft_data, cpu);
+		sfd = rcu_dereference(*per_cpu_ptr(&sft_data, cpu));
 
 		/* Use ARCH provided counters whenever possible */
 		if (!sfd || sfd->source != SCALE_FREQ_SOURCE_ARCH) {
-			per_cpu(sft_data, cpu) = data;
+			rcu_assign_pointer(per_cpu(sft_data, cpu), data);
 			cpumask_set_cpu(cpu, &scale_freq_counters_mask);
 		}
 	}
 
+	rcu_read_unlock();
+
 	update_scale_freq_invariant(true);
 }
 EXPORT_SYMBOL_GPL(topology_set_scale_freq_source);

@@ -86,22 +91,32 @@ void topology_clear_scale_freq_source(enum scale_freq_source source,
 	struct scale_freq_data *sfd;
 	int cpu;
 
+	rcu_read_lock();
+
 	for_each_cpu(cpu, cpus) {
-		sfd = per_cpu(sft_data, cpu);
+		sfd = rcu_dereference(*per_cpu_ptr(&sft_data, cpu));
 
 		if (sfd && sfd->source == source) {
-			per_cpu(sft_data, cpu) = NULL;
+			rcu_assign_pointer(per_cpu(sft_data, cpu), NULL);
 			cpumask_clear_cpu(cpu, &scale_freq_counters_mask);
 		}
 	}
 
+	rcu_read_unlock();
+
+	/*
+	 * Make sure all references to previous sft_data are dropped to avoid
+	 * use-after-free races.
+	 */
+	synchronize_rcu();
+
 	update_scale_freq_invariant(false);
 }
 EXPORT_SYMBOL_GPL(topology_clear_scale_freq_source);
 
 void topology_scale_freq_tick(void)
 {
-	struct scale_freq_data *sfd = *this_cpu_ptr(&sft_data);
+	struct scale_freq_data *sfd = rcu_dereference_sched(*this_cpu_ptr(&sft_data));
 
 	if (sfd)
 		sfd->set_freq_scale();

Signature · 1 line

2.31.1.272.g89b43f80a514

The Frequency Invariance Engine (FIE) is providing a frequency scaling correction factor that helps achieve more accurate load-tracking.

Quoted 2 lines · Viresh Kumar

The Frequency Invariance Engine (FIE) is providing a frequency scaling correction factor that helps achieve more accurate load-tracking.

[..]

drivers/cpufreq/cppc_cpufreq.c static struct cppc_workaround_oem_info wa_info[] = { · line 202

Quoted 13 lines

drivers/cpufreq/cppc_cpufreq.c +13 -0

+static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
+{
+	struct cppc_freq_invariance *cppc_fi;
+	int cpu;
+
+	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
+		return;
+
+	/* policy->cpus will be empty here, use related_cpus instead */
+	topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, policy->related_cpus);
+
+	for_each_cpu(cpu, policy->related_cpus) {
+		cppc_fi = &per_cpu(cppc_freq_inv, cpu);

Do you think it might be worth having here something like:

		if (!cppc_fi->cpu_data)
			continue;

This would be to protect against cases where the platform does not boot with all CPUs or the module is loaded after some have already been offlined. Unlikely, but..

drivers/cpufreq/cppc_cpufreq.c static struct cppc_workaround_oem_info wa_info[] = { · line 206

Quoted 4 lines

drivers/cpufreq/cppc_cpufreq.c +4 -0

+		irq_work_sync(&cppc_fi->irq_work);
+		kthread_cancel_work_sync(&cppc_fi->work);
+	}
+}

The rest of the code is almost the same as the original, so that is all from me :).

Thanks,
Ionela.

Quoted 23 lines · Ionela Voinescu

Quoted 2 lines · Viresh Kumar

The Frequency Invariance Engine (FIE) is providing a frequency scaling correction factor that helps achieve more accurate load-tracking.

[..]

drivers/cpufreq/cppc_cpufreq.c static struct cppc_workaround_oem_info wa_info[] = { · line 202

Quoted 13 lines

drivers/cpufreq/cppc_cpufreq.c +13 -0

+static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
+{
+	struct cppc_freq_invariance *cppc_fi;
+	int cpu;
+
+	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
+		return;
+
+	/* policy->cpus will be empty here, use related_cpus instead */
+	topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, policy->related_cpus);
+
+	for_each_cpu(cpu, policy->related_cpus) {
+		cppc_fi = &per_cpu(cppc_freq_inv, cpu);

Do you think it might be worth having here something like:

		if (!cppc_fi->cpu_data)
			continue;

Quoted 3 lines

This would be to protect against cases where the platform does not boot with all CPUs or the module is loaded after some have already been offlined. Unlikely, but..

Even in that case policy->cpus will contain all offline+online CPUs (at ->init() time), isn't it ?

Quoted 10 lines

drivers/cpufreq/cppc_cpufreq.c static struct cppc_workaround_oem_info wa_info[] = { · line 206

Quoted 4 lines

drivers/cpufreq/cppc_cpufreq.c +4 -0

+		irq_work_sync(&cppc_fi->irq_work);
+		kthread_cancel_work_sync(&cppc_fi->work);
+	}
+}

The rest of the code is almost the same as the original, so that is all from me :).

Thanks,
Ionela.

Signature · 1 line

viresh

Hey,

Quoted 28 lines · Viresh Kumar

Quoted 26 lines · Ionela Voinescu

Quoted 2 lines · Viresh Kumar

The Frequency Invariance Engine (FIE) is providing a frequency scaling correction factor that helps achieve more accurate load-tracking.

[..]

drivers/cpufreq/cppc_cpufreq.c static struct cppc_workaround_oem_info wa_info[] = { · line 202

Quoted 13 lines

drivers/cpufreq/cppc_cpufreq.c +13 -0

+static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
+{
+	struct cppc_freq_invariance *cppc_fi;
+	int cpu;
+
+	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
+		return;
+
+	/* policy->cpus will be empty here, use related_cpus instead */
+	topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, policy->related_cpus);
+
+	for_each_cpu(cpu, policy->related_cpus) {
+		cppc_fi = &per_cpu(cppc_freq_inv, cpu);

Do you think it might be worth having here something like:

		if (!cppc_fi->cpu_data)
			continue;

This would be to protect against cases where the platform does not boot with all CPUs or the module is loaded after some have already been offlined. Unlikely, but..

Quoted 3 lines

Even in that case policy->cpus will contain all offline+online CPUs (at ->init() time), isn't it ?

Right, my bad. I missed cpumask_and(policy->cpus, policy->cpus, cpu_online_mask) being done after init(). It logically seems a bit wrong, but drivers are in control of setting policy->cpus and acting on it, and in this case the driver does the right thing.

Quoted 35 lines · Ionela Voinescu

Hey,

Quoted 31 lines · Viresh Kumar

Quoted 20 lines · Ionela Voinescu

Quoted 2 lines · Viresh Kumar

The Frequency Invariance Engine (FIE) is providing a frequency scaling correction factor that helps achieve more accurate load-tracking.

[..]

drivers/cpufreq/cppc_cpufreq.c static struct cppc_workaround_oem_info wa_info[] = { · line 202

Quoted 13 lines

drivers/cpufreq/cppc_cpufreq.c +13 -0

+static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
+{
+	struct cppc_freq_invariance *cppc_fi;
+	int cpu;
+
+	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
+		return;
+
+	/* policy->cpus will be empty here, use related_cpus instead */
+	topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, policy->related_cpus);
+
+	for_each_cpu(cpu, policy->related_cpus) {
+		cppc_fi = &per_cpu(cppc_freq_inv, cpu);

Do you think it might be worth having here something like:

Quoted 6 lines

		if (!cppc_fi->cpu_data)
			continue;

This would be to protect against cases where the platform does not boot with all CPUs or the module is loaded after some have already been offlined. Unlikely, but..

Even in that case policy->cpus will contain all offline+online CPUs (at ->init() time), isn't it ?

Quoted 4 lines

Right, my bad. I missed cpumask_and(policy->cpus, policy->cpus, cpu_online_mask) being done after init(). It logically seems a bit wrong, but drivers are in control of setting policy->cpus and acting on it, and in this case the driver does the right thing.

Do you want me to re-add your Reviewed-by here ?

drivers/cpufreq/cppc_cpufreq.c +11 -0

+static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
+{
+	struct cppc_freq_invariance *cppc_fi;
+	int cpu, ret;
+
+	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
+		return;
+
+	for_each_cpu(cpu, policy->cpus) {
+		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
+		cppc_fi->cpu = cpu;

drivers/cpufreq/cppc_cpufreq.c +5 -0

+	}
+
+	/* Register for freq-invariance */
+	topology_set_scale_freq_source(&cppc_sftd, policy->cpus);
+}

Signature · 2 lines

-- 
viresh

To be honest I would like to have more time on this before you merge the set, to better understand Qian's results and some observations I have for Thunder X2 (I will share in a bit).

For the code, I think it's fine. I have a single observation regarding the following code:

Quoted 11 lines

(quoted diff) +11 -0

+static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
+{
+	struct cppc_freq_invariance *cppc_fi;
+	int cpu, ret;
+
+	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
+		return;
+
+	for_each_cpu(cpu, policy->cpus) {
+		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
+		cppc_fi->cpu = cpu;

Review

Quoted 10 lines

(quoted diff) +10 -0

+		cppc_fi->cpu_data = policy->driver_data;
+		kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
+		init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
+
+		ret = cppc_get_perf_ctrs(cpu, &cppc_fi->prev_perf_fb_ctrs);
+		if (ret) {
+			pr_warn("%s: failed to read perf counters for cpu:%d: %d\n",
+				__func__, cpu, ret);
+			return;
+		}

For this condition above, think about a scenario where reading counters for offline CPUs returns an error. I'm not sure if that can happen, to be honest. That would mean here that you will never initialise the freq source unless all CPUs in the policy are online at policy creation.

My recommendation is to warn about the failed read of perf counters but only return from this function if the target CPU is online as well when reading counters fails.

This is probably a nit, so I'll let you decide if you want to do something about this.

Quoted 3 lines · Ionela Voinescu

To be honest I would like to have more time on this before you merge the set, to better understand Qian's results and some observations I have for Thunder X2 (I will share in a bit).

Ideally, this code was already merged in 5.13 and would have required us to fix any problems as we encounter them. I did revert it because it caused a kernel crash and I wasn't sure if there was a sane/easy way of fixing that so late in the release cycle. That was the right thing to do then.

All those issues are gone now, we may have an issue around rounding of counters or some hardware specific issues, it isn't clear yet.

But the stuff works fine otherwise, doesn't make the kernel crash and it is controlled with a CONFIG_ option, so those who don't want to use it can still disable it.

The merge window is here now, if we don't merge it now, it gets delayed by a full cycle (roughly two months) and if we merge it now and are able to narrow down the rounding issues, if there are any, we will have full two months to make a fix for that and still push it in 5.14 itself.

And so I would like to get it merged in this merge window itself, it also makes sure more people would get to test it, like Qian was able to figure out a problem here for us.

Quoted 30 lines

For the code, I think it's fine. I have a single observation regarding the following code:

Quoted 11 lines

(quoted diff) +11 -0

+static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
+{
+	struct cppc_freq_invariance *cppc_fi;
+	int cpu, ret;
+
+	if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
+		return;
+
+	for_each_cpu(cpu, policy->cpus) {
+		cppc_fi = &per_cpu(cppc_freq_inv, cpu);
+		cppc_fi->cpu = cpu;

Review

Quoted 10 lines

(quoted diff) +10 -0

+		cppc_fi->cpu_data = policy->driver_data;
+		kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
+		init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
+
+		ret = cppc_get_perf_ctrs(cpu, &cppc_fi->prev_perf_fb_ctrs);
+		if (ret) {
+			pr_warn("%s: failed to read perf counters for cpu:%d: %d\n",
+				__func__, cpu, ret);
+			return;
+		}

For this condition above, think about a scenario where reading counters for offline CPUs returns an error. I'm not sure if that can happen, to be honest. That would mean here that you will never initialise the freq source unless all CPUs in the policy are online at policy creation.

Quoted 6 lines

My recommendation is to warn about the failed read of perf counters but only return from this function if the target CPU is online as well when reading counters fails.

This is probably a nit, so I'll let you decide if you want to do something about this.

That is a very good observation actually. Thanks for that. This is how I fixed it.

drivers/cpufreq/cppc_cpufreq.c +0 -1

index d688877e8fbe..f6540068d0fe 100644
@@ -171,7 +171,13 @@ static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
                if (ret) {
                        pr_warn("%s: failed to read perf counters for cpu:%d: %d\n",
                                __func__, cpu, ret);
-                       return;

viresh

Okay, makes sense. I have not seen this code actually do anything wrong so far, and the issues I see on ThunderX2 point more to misbehaving counters for this purpose. This being said, I would have probably preferred for this feature to be disabled by default, until we've tested more, but that won't give the chance to anyone else to test.

Thanks!

Reviewed-by Ionela Voinescu <[email protected]>

CPPC cpufreq driver is used for ARM servers and this patch series tries to provide counter-based frequency invariance support for them in the absence for architecture specific counters (like AMUs).

Viresh, this series works fine on my quick tests so far.

BTW, I
noticed some strange things even with the series applied mentioned below when reading acpi_cppc vs cpufreq sysfs. Do you happen to know are those just hardware/firmware issues because Linux just faithfully exported the register values?

== Arm64 server Foo ==

CPU max MHz:                     3000.0000
CPU min MHz:                     1000.0000

/sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf
300
/sys/devices/system/cpu/cpu0/acpi_cppc/lowest_freq
1000 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_nonlinear_perf 200
/sys/devices/system/cpu/cpu0/acpi_cppc/lowest_perf
100
/sys/devices/system/cpu/cpu0/acpi_cppc/nominal_freq <--- should be 3000? 2800
/sys/devices/system/cpu/cpu0/acpi_cppc/nominal_perf <--- should be 300? 280

/sys/devices/system/cpu/cpu0/acpi_cppc/reference_perf 100

== Arm64 server Bar ==

CPU max MHz:                     3000.0000
CPU min MHz:                     375.0000

/sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf <--- should be 3000? There is no cpufreq boost. 3300

/sys/devices/system/cpu/cpu0/acpi_cppc/lowest_freq <--- don't understand why 0. 0

/sys/devices/system/cpu/cpu0/acpi_cppc/lowest_nonlinear_perf 375 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_perf 375 /sys/devices/system/cpu/cpu0/acpi_cppc/nominal_freq <--- ditto 0 /sys/devices/system/cpu/cpu0/acpi_cppc/nominal_perf 3000 /sys/devices/system/cpu/cpu0/acpi_cppc/reference_perf 100

Quoted 3 lines · Viresh Kumar

CPPC cpufreq driver is used for ARM servers and this patch series tries to provide counter-based frequency invariance support for them in the absence for architecture specific counters (like AMUs).

Viresh, this series works fine on my quick tests so far.

Quoted 8 lines · Qian Cai

Quoted 3 lines · Viresh Kumar

CPPC cpufreq driver is used for ARM servers and this patch series tries to provide counter-based frequency invariance support for them in the absence for architecture specific counters (like AMUs).

Viresh, this series works fine on my quick tests so far.

Do you want me to add your Tested-by for the series ?

Viresh, I am afraid I don't feel comfortable yet. I have a few new tests in development, and will provide an update once ready.

Also, I noticed the delivered perf is even smaller than lowest_perf (100).

# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103377547901 del:54540736873
# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103379170101 del:54541599117

100 * (54541599117 - 54540736873) / (103379170101 - 103377547901) = 53

My understanding is that the delivered perf should fail into the range between lowest_perf and highest_perf. Is that assumption correct? This happens on 5.4-based kernel, so I am in process running your series on that system to see if there is any differences. In any case, if it is a bug it is pre-existing, but I'd like to understand a bit better in that front first.

Quoted 2 lines · Qian Cai

Viresh, I am afraid I don't feel comfortable yet. I have a few new tests in development, and will provide an update once ready.

Oh sure, np.

Quoted 8 lines

Also, I noticed the delivered perf is even smaller than lowest_perf (100).

# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103377547901 del:54540736873
# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103379170101 del:54541599117

100 * (54541599117 - 54540736873) / (103379170101 - 103377547901) = 53

Quoted 6 lines

My understanding is that the delivered perf should fail into the range between lowest_perf and highest_perf. Is that assumption correct? This happens on 5.4-based kernel, so I am in process running your series on that system to see if there is any differences. In any case, if it is a bug it is pre-existing, but I'd like to understand a bit better in that front first.

Vincent:

Can that happen because of CPU idle ?

--
viresh

Quoted 14 lines · Viresh Kumar

Quoted 2 lines · Qian Cai

Viresh, I am afraid I don't feel comfortable yet. I have a few new tests in development, and will provide an update once ready.

Oh sure, np.

Quoted 7 lines

Also, I noticed the delivered perf is even smaller than lowest_perf (100).

# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103377547901 del:54540736873
# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103379170101 del:54541599117

Quoted 1 line

Quoted 1 line

100 * (54541599117 - 54540736873) / (103379170101 - 103377547901) = 53

I'm not sure that I understand your point. The formula above says that cpu8 run @ 53% of nominal performance

Quoted 11 lines

Quoted 6 lines

My understanding is that the delivered perf should fail into the range between lowest_perf and highest_perf. Is that assumption correct? This happens on 5.4-based kernel, so I am in process running your series on that system to see if there is any differences. In any case, if it is a bug it is pre-existing, but I'd like to understand a bit better in that front first.

Vincent:

Can that happen because of CPU idle ?

Quoted 2 lines

--
viresh

Hi guys,

Quoted 15 lines · Vincent Guittot

Quoted 14 lines · Viresh Kumar

Quoted 2 lines · Qian Cai

Viresh, I am afraid I don't feel comfortable yet. I have a few new tests in development, and will provide an update once ready.

Oh sure, np.

Quoted 7 lines

Also, I noticed the delivered perf is even smaller than lowest_perf (100).

# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103377547901 del:54540736873
# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103379170101 del:54541599117

Quoted 5 lines

Quoted 1 line

Quoted 1 line

100 * (54541599117 - 54540736873) / (103379170101 - 103377547901) = 53

I'm not sure that I understand your point. The formula above says that cpu8 run @ 53% of nominal performance

I think this is based on a previous example Qian had where:

/sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf 300 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_freq 1000 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_perf 100 /sys/devices/system/cpu/cpu0/acpi_cppc/reference_perf 100

..so the 100 is not from obtaining percentage, is the reference performance.

The logic of the formula is to obtain the delivered performance when knowing the number of ticks for each counter, so:

So if one gets (103379170101 - 103377547901) ticks for the counter at running at 1GHz(perf 100), what is the frequency of the core, if its counter ticked (54541599117 - 54540736873) times in the same interval of time?

The answer is 530MHz(perf 53), which is lower than the lowest frequency at 1GHz(perf 100).

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

My understanding is that the delivered perf should fail into the range between lowest_perf and highest_perf. Is that assumption correct? This happens on 5.4-based kernel, so I am in process running your series on that system to see if there is any differences. In any case, if it is a bug it is pre-existing, but I'd like to understand a bit better in that front first.

Quoted 4 lines

Quoted 4 lines

Vincent:

Can that happen because of CPU idle ?

Not if the counters are implemented properly. The kernel considers that both reference and delivered performance counters should stop or reset during idle. The kernel would not account for idle itself.

If the reference performance counter does not stop during idle, while the core performance counter (delivered) does stop, the behavior above should be seen very often.

Qian, do you see these small delivered performance values often or seldom?

Thanks,
Ionela.

Quoted 2 lines

Quoted 2 lines

--
viresh

Quoted 47 lines · Ionela Voinescu

Hi guys,

Quoted 15 lines · Vincent Guittot

Quoted 14 lines · Viresh Kumar

Quoted 2 lines · Qian Cai

Viresh, I am afraid I don't feel comfortable yet. I have a few new tests in development, and will provide an update once ready.

Oh sure, np.

Quoted 7 lines

Also, I noticed the delivered perf is even smaller than lowest_perf (100).

# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103377547901 del:54540736873
# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103379170101 del:54541599117

Quoted 5 lines

Quoted 1 line

Quoted 1 line

100 * (54541599117 - 54540736873) / (103379170101 - 103377547901) = 53

I'm not sure that I understand your point. The formula above says that cpu8 run @ 53% of nominal performance

I think this is based on a previous example Qian had where:

/sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf 300 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_freq 1000 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_perf 100 /sys/devices/system/cpu/cpu0/acpi_cppc/reference_perf 100

..so the 100 is not from obtaining percentage, is the reference performance.

The logic of the formula is to obtain the delivered performance when knowing the number of ticks for each counter, so:

So if one gets (103379170101 - 103377547901) ticks for the counter at running at 1GHz(perf 100), what is the frequency of the core, if its counter ticked (54541599117 - 54540736873) times in the same interval of time?

Quoted 2 lines

The answer is 530MHz(perf 53), which is lower than the lowest frequency at 1GHz(perf 100).

But the nominal_perf is 280 and not 100 if i'm not wrong so the perf value is 148 > lowest_perf in this case

Quoted 31 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

My understanding is that the delivered perf should fail into the range between lowest_perf and highest_perf. Is that assumption correct? This happens on 5.4-based kernel, so I am in process running your series on that system to see if there is any differences. In any case, if it is a bug it is pre-existing, but I'd like to understand a bit better in that front first.

Quoted 4 lines

Quoted 4 lines

Vincent:

Can that happen because of CPU idle ?

Not if the counters are implemented properly. The kernel considers that both reference and delivered performance counters should stop or reset during idle. The kernel would not account for idle itself.

If the reference performance counter does not stop during idle, while the core performance counter (delivered) does stop, the behavior above should be seen very often.

Qian, do you see these small delivered performance values often or seldom?

Thanks,
Ionela.

Quoted 2 lines

Quoted 2 lines

--
viresh

Quoted 48 lines · Vincent Guittot

Quoted 47 lines · Ionela Voinescu

Hi guys,

Quoted 15 lines · Vincent Guittot

Quoted 14 lines · Viresh Kumar

Quoted 2 lines · Qian Cai

Viresh, I am afraid I don't feel comfortable yet. I have a few new tests in development, and will provide an update once ready.

Oh sure, np.

Quoted 7 lines

Also, I noticed the delivered perf is even smaller than lowest_perf (100).

# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103377547901 del:54540736873
# cat /sys/devices/system/cpu/cpu8/acpi_cppc/feedback_ctrs
 ref:103379170101 del:54541599117

Quoted 5 lines

Quoted 1 line

Quoted 1 line

100 * (54541599117 - 54540736873) / (103379170101 - 103377547901) = 53

I'm not sure that I understand your point. The formula above says that cpu8 run @ 53% of nominal performance

I think this is based on a previous example Qian had where:

/sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf 300 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_freq 1000 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_perf 100 /sys/devices/system/cpu/cpu0/acpi_cppc/reference_perf 100

..so the 100 is not from obtaining percentage, is the reference performance.

The logic of the formula is to obtain the delivered performance when knowing the number of ticks for each counter, so:

So if one gets (103379170101 - 103377547901) ticks for the counter at running at 1GHz(perf 100), what is the frequency of the core, if its counter ticked (54541599117 - 54540736873) times in the same interval of time?

Quoted 6 lines

Quoted 2 lines

The answer is 530MHz(perf 53), which is lower than the lowest frequency at 1GHz(perf 100).

But the nominal_perf is 280 and not 100 if i'm not wrong so the perf value is 148 > lowest_perf in this case

Nominal performance has no meaning here. The reference counter ticks with the frequency equivalent to reference performance.

Nominal performance is the maximum performance when !boost. Highest performance is the maximum performance available including boost frequencies. So nominal performance has no impact in these translations from counter values to delivered performance.

Hope it helps,
Ionela.

Quoted 31 lines

Quoted 31 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

My understanding is that the delivered perf should fail into the range between lowest_perf and highest_perf. Is that assumption correct? This happens on 5.4-based kernel, so I am in process running your series on that system to see if there is any differences. In any case, if it is a bug it is pre-existing, but I'd like to understand a bit better in that front first.

Quoted 4 lines

Quoted 4 lines

Vincent:

Can that happen because of CPU idle ?

Not if the counters are implemented properly. The kernel considers that both reference and delivered performance counters should stop or reset during idle. The kernel would not account for idle itself.

If the reference performance counter does not stop during idle, while the core performance counter (delivered) does stop, the behavior above should be seen very often.

Qian, do you see these small delivered performance values often or seldom?

Thanks,
Ionela.

Quoted 2 lines

Quoted 2 lines

--
viresh

Not if the counters are implemented properly. The kernel considers that both reference and delivered performance counters should stop or reset during idle. The kernel would not account for idle itself.

If the reference performance counter does not stop during idle, while the core performance counter (delivered) does stop, the behavior above should be seen very often.

Qian, do you see these small delivered performance values often or seldom?

Quoted 10 lines · Ionela Voinescu

Not if the counters are implemented properly. The kernel considers that both reference and delivered performance counters should stop or reset during idle. The kernel would not account for idle itself.

If the reference performance counter does not stop during idle, while the core performance counter (delivered) does stop, the behavior above should be seen very often.

Qian, do you see these small delivered performance values often or seldom?

Ionela, so I managed to upgrade the kernel on the system to today's linux-next which suppose to include this series. The delivered perf is now 280. However, scaling_min_freq (200 MHz) is not equal to lowest_perf (100).

scaling_driver: acpi_cppc

scaling_governor: schedutil

Is that normal because lowest_nonlinear_perf is 200?

Also, on this pretty idle system, 158 of 160 CPUs are always running in max freq (280 MHz). The other 2 are running in 243 and 213 MHz according to scaling_cur_freq. Apparently, "schedutil" does not work proper on this system. I am going to try other governors to narrow down the issue a bit.

FYI, here is the acpi_cppc registers reading:

/sys/devices/system/cpu/cpu0/acpi_cppc/feedback_ctrs ref:160705801 del:449594095 /sys/devices/system/cpu/cpu0/acpi_cppc/highest_perf 300 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_freq 1000 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_nonlinear_perf 200 /sys/devices/system/cpu/cpu0/acpi_cppc/lowest_perf 100 /sys/devices/system/cpu/cpu0/acpi_cppc/nominal_freq 2800 /sys/devices/system/cpu/cpu0/acpi_cppc/nominal_perf 280 /sys/devices/system/cpu/cpu0/acpi_cppc/reference_perf 100 /sys/devices/system/cpu/cpu0/acpi_cppc/wraparound_time 18446744073709551615

Quoted 1 line

scaling_driver: acpi_cppc

                  ^^^^^^^^^

I suppose you mean "cppc-cpufreq"?

"acpi_cppc" is not a scaling driver option.

So your CPUs run at frequencies between 200MHz and 280MHz?

Based on your acpi_cppc information below I would have assumed 2GHz as lowest nonlinear and 2.8GHz as nominal. The reason for this is that according to the ACPI spec the frequency values in the _CPC objects are supposed to be in MHz, so 2800 MHz for nominal frequency would be 2.8GHz.

When you try more governors, make sure to check out the difference between scaling_cur_freq and cpuinfo_cur_freq at [2]. The first gives you the frequency that the governor (schedutil) is asking for, while the second is giving you the current frequency obtained from the counters.

So to check the actual frequency the cores are running at, please check cpuinfo_cur_freq.

Quoted 4 lines · Ionela Voinescu

Quoted 1 line

scaling_driver: acpi_cppc

                  ^^^^^^^^^

I suppose you mean "cppc-cpufreq"?

Quoted 1 line

"acpi_cppc" is not a scaling driver option.

Ionela, yes. Sorry about that.

Quoted 1 line

So your CPUs run at frequencies between 200MHz and 280MHz?

2000 to 2800 MHz.

Quoted 11 lines

Based on your acpi_cppc information below I would have assumed 2GHz as lowest nonlinear and 2.8GHz as nominal. The reason for this is that according to the ACPI spec the frequency values in the _CPC objects are supposed to be in MHz, so 2800 MHz for nominal frequency would be 2.8GHz.

When you try more governors, make sure to check out the difference between scaling_cur_freq and cpuinfo_cur_freq at [2]. The first gives you the frequency that the governor (schedutil) is asking for, while the second is giving you the current frequency obtained from the counters.

Quoted 2 lines

So to check the actual frequency the cores are running at, please check cpuinfo_cur_freq.

The problem is that all CPUs are never scaling down. "cpuinfo_cur_freq" and "scaling_cur_freq" are always the 2800 MHz on all CPUs on this idle system. This looks like a regression somewhere as in 5.4-based kernel, I can see "cpuinfo_cur_freq" can go down to 2000 MHz in the same scenario. I'll bisect a bit unless you have better ideas?

Quick questions for you:

1. When you say you tried a 5.4 kernel, did you try it with these patches backported? They also have some dependencies with the recent changes in the arch topology driver and cpufreq so they would not be straight forward to backport.

If the 5.4 kernel you tried did not have these patches, it might be best to try next/master that has these patches, but with CONFIG_ACPI_CPPC_CPUFREQ_FIE=n, just to eliminate the possibility that an incorrect frequency scale factor here would affect utilization that would then affect the schedutil frequency selection. I would not expect this behavior even if the scale factor was wrong, but it would be good to rule out.

2. Is your platform booting with all CPUs? Are any hotplug operations done in your scenario?

Quick questions for you:

1. When you say you tried a 5.4 kernel, did you try it with these patches backported? They also have some dependencies with the recent changes in the arch topology driver and cpufreq so they would not be straight forward to backport.

If the 5.4 kernel you tried did not have these patches, it might be best to try next/master that has these patches, but with CONFIG_ACPI_CPPC_CPUFREQ_FIE=n, just to eliminate the possibility that an incorrect frequency scale factor here would affect utilization that would then affect the schedutil frequency selection. I would not expect this behavior even if the scale factor was wrong, but it would be good to rule out.

2. Is your platform booting with all CPUs? Are any hotplug operations done in your scenario?

Ionela, I found that set ACPI_PROCESSOR=y instead of ACPI_PROCESSOR=m will fix the previous mentioned issues here (any explanations of that?) even though the scaling down is not perfect. Now, we have the following on this idle system:

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c

(quoted diff) +0 -0

 	79 1000000
  	1 1160000
 	73 1400000
  	1 2000000
  	4 2010000
  	1 2800000
  	1 860000

Even if I rerun a few times, there could still have a few CPUs running lower than lowest_perf (1GHz). Also, even though I set all CPUs to use "userspace" governor and set freq to the lowest. A few CPUs keep changing at will.

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c
	156 1000000
  	3 2000000
  	1 760000

Another date point is that set ACPI_CPPC_CPUFREQ_FIE=n fixed the issue that any CPU could run below the lowest freq.

schedutil:

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c
     80 1000000
     78 1400000
      1 2010000
      1 2800000

userspace:

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c
    158 1000000
      2 2000000

Ionela, I found that set ACPI_PROCESSOR=y instead of ACPI_PROCESSOR=m will fix the previous mentioned issues here (any explanations of that?) even though the scaling down is not perfect.

Now, we have the following on this idle system:

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c

(quoted diff) +0 -0

 	79 1000000
  	1 1160000
 	73 1400000
  	1 2000000
  	4 2010000
  	1 2800000
  	1 860000

Even if I rerun a few times, there could still have a few CPUs running lower than lowest_perf (1GHz).

Also, even though I set all CPUs to use "userspace" governor and set freq to the lowest. A few CPUs keep changing at will.

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c
	156 1000000
  	3 2000000
  	1 760000

Quoted 15 lines · Ionela Voinescu

Quick questions for you:

1. When you say you tried a 5.4 kernel, did you try it with these patches backported? They also have some dependencies with the recent changes in the arch topology driver and cpufreq so they would not be straight forward to backport.

If the 5.4 kernel you tried did not have these patches, it might be best to try next/master that has these patches, but with CONFIG_ACPI_CPPC_CPUFREQ_FIE=n, just to eliminate the possibility that an incorrect frequency scale factor here would affect utilization that would then affect the schedutil frequency selection. I would not expect this behavior even if the scale factor was wrong, but it would be good to rule out.

Quoted 2 lines

2. Is your platform booting with all CPUs? Are any hotplug operations done in your scenario?

Ionela, I found that set ACPI_PROCESSOR=y instead of ACPI_PROCESSOR=m will fix the previous mentioned issues here (any explanations of that?) even though the scaling down is not perfect. Now, we have the following on this idle system:

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c

(quoted diff) +0 -0

 	79 1000000
  	1 1160000
 	73 1400000
  	1 2000000
  	4 2010000
  	1 2800000
  	1 860000

Even if I rerun a few times, there could still have a few CPUs running lower than lowest_perf (1GHz). Also, even though I set all CPUs to use "userspace" governor and set freq to the lowest. A few CPUs keep changing at will.

# cat /sys/devices/system/cpu/*/cpufreq/cpuinfo_cur_freq | sort | uniq  -c
	156 1000000
  	3 2000000
  	1 760000

Quoted 5 lines

Ionela, I found that set ACPI_PROCESSOR=y instead of ACPI_PROCESSOR=m will fix the previous mentioned issues here (any explanations of that?) even though the scaling down is not perfect. Now, we have the following on this idle system:

I don't see how this would have played a role. The cppc cpufreq driver depends on functionality gated by CONFIG_ACPI_CPPC_LIB which in turn needs CONFIG_ACPI_PROCESSOR to trigger the parsing of the _CPC objects. If CONFIG_ACPI_PROCESSOR functionality is not built in or loaded, the cppc cpufreq driver could not be used at all.

The problem is that all CPUs are never scaling down. "cpuinfo_cur_freq" and "scaling_cur_freq" are always the 2800 MHz on all CPUs on this idle system. This looks like a regression somewhere as in 5.4-based kernel, I can see "cpuinfo_cur_freq" can go down to 2000 MHz in the same scenario. I'll bisect a bit unless you have better ideas?

Not if the counters are implemented properly. The kernel considers that both reference and delivered performance counters should stop or reset during idle. The kernel would not account for idle itself.

If the reference performance counter does not stop during idle, while the core performance counter (delivered) does stop, the behavior above should be seen very often.

Qian, do you see these small delivered performance values often or seldom?

Hello,

Changes since V2:

- We don't need start_cpu() and stop_cpu() callbacks anymore, we can make it work using policy ->init() and exit() alone.

- Two new cleanup patches 1/4 and 2/4.

- Improved commit log of 3/4.

- Dropped WARN_ON(local_freq_scale > 1024), since this can occur on counter's overlap (seen with Vincent's setup).

This is tested on my Hikey platform (without the actual read/write to performance counters), with this script for over an hour:

while true; do

    for i in `seq 1 7`;
    do
        echo 0 > /sys/devices/system/cpu/cpu$i/online;
    done;

    for i in `seq 1 7`;
    do
        echo 1 > /sys/devices/system/cpu/cpu$i/online;
    done;

done

The same is done by Vincent on ThunderX2 and no issues were seen.

Hi guys,

Quoted 7 lines · Viresh Kumar

Hello,

Changes since V2:

- We don't need start_cpu() and stop_cpu() callbacks anymore, we can make it work using policy ->init() and exit() alone.

Quoted 7 lines

- Two new cleanup patches 1/4 and 2/4.

- Improved commit log of 3/4.

- Dropped WARN_ON(local_freq_scale > 1024), since this can occur on counter's overlap (seen with Vincent's setup).

If you happen to have the data around, I would like to know more about your observations on ThunderX2.

I tried ThunderX2 as well, with the following observations:

Booting with userspace governor and all CPUs online, the CPPC frequency scale factor was all over the place (even much larger than 1024).

My initial assumptions:
- Counters do not behave properly in light of SMT
- Firmware does not do a good job to keep the reference and core counters monotonic: save and restore at core off.

So I offlined all CPUs with the exception of 0, 32, 64, 96 - threads of a single core (part of policy0). With this all works very well:

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1056000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1863.095370] CPU96: cppc scale: 697.
[ 1863.175370] CPU0: cppc scale: 492.
[ 1863.215367] CPU64: cppc scale: 492.
[ 1863.235366] CPU96: cppc scale: 492.
[ 1863.485368] CPU32: cppc scale: 492.

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1936000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1891.395363] CPU96: cppc scale: 558.
[ 1891.415362] CPU0: cppc scale: 595.
[ 1891.435362] CPU32: cppc scale: 615.
[ 1891.465363] CPU96: cppc scale: 635.
[ 1891.495361] CPU0: cppc scale: 673.
[ 1891.515360] CPU32: cppc scale: 703.
[ 1891.545360] CPU96: cppc scale: 738.
[ 1891.575360] CPU0: cppc scale: 779.
[ 1891.605360] CPU96: cppc scale: 829.
[ 1891.635360] CPU0: cppc scale: 879.

root@target:/sys/devices/system/cpu/cpufreq/policy0#

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 2200000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1896.585363] CPU32: cppc scale: 1004.
[ 1896.675359] CPU64: cppc scale: 973.
[ 1896.715359] CPU0: cppc scale: 1024.

I'm doing a rate limited printk only for increase/decrease values over 64 in the scale factor value.

This showed me that SMT is handled properly.

Then, as soon as I start onlining CPUs 1, 33, 65, 97, the scale factor stops being even close to correct, for example:

[238394.770328] CPU96: cppc scale: 22328.
[238395.628846] CPU96: cppc scale: 245.
[238516.087115] CPU96: cppc scale: 930.
[238523.385009] CPU96: cppc scale: 245.
[238538.767473] CPU96: cppc scale: 936.
[238538.867546] CPU96: cppc scale: 245.
[238599.367932] CPU97: cppc scale: 2728.
[238599.859865] CPU97: cppc scale: 452.
[238647.786284] CPU96: cppc scale: 1438.
[238669.604684] CPU96: cppc scale: 27306.
[238676.805049] CPU96: cppc scale: 245.
[238737.642902] CPU97: cppc scale: 2035.
[238737.664995] CPU97: cppc scale: 452.
[238788.066193] CPU96: cppc scale: 2749.
[238788.110192] CPU96: cppc scale: 245.
[238817.231659] CPU96: cppc scale: 2698.
[238818.083687] CPU96: cppc scale: 245.
[238845.466850] CPU97: cppc scale: 2990.
[238847.477805] CPU97: cppc scale: 452.
[238936.984107] CPU97: cppc scale: 1590.
[238937.029079] CPU97: cppc scale: 452.
[238979.052464] CPU97: cppc scale: 911.
[238980.900668] CPU97: cppc scale: 452.
[239149.587889] CPU96: cppc scale: 803.
[239151.085516] CPU96: cppc scale: 245.

[239303.871373] CPU64: cppc scale: 956.
[239303.906837] CPU64: cppc scale: 245.
[239308.666786] CPU96: cppc scale: 821.
[239319.440634] CPU96: cppc scale: 245.
[239389.978395] CPU97: cppc scale: 4229.
[239391.969562] CPU97: cppc scale: 452.
[239415.894738] CPU96: cppc scale: 630.
[239417.875326] CPU96: cppc scale: 245.

The counter values shown by feedback_ctrs do not seem monotonic even when only core 0 threads are online.

ref:2812420736 del:166051103 ref:3683620736 del:641578595 ref:1049653440 del:1548202980 ref:2099053440 del:2120997459 ref:3185853440 del:2714205997 ref:712486144 del:3708490753 ref:3658438336 del:3401357212 ref:1570998080 del:2279728438

For now I was just wondering if you have seen the same and whether you have an opinion on this.

Quoted 13 lines

This is tested on my Hikey platform (without the actual read/write to performance counters), with this script for over an hour:

while true; do

    for i in `seq 1 7`;
    do
        echo 0 > /sys/devices/system/cpu/cpu$i/online;
    done;

    for i in `seq 1 7`;
    do
        echo 1 > /sys/devices/system/cpu/cpu$i/online;
    done;

Quoted 4 lines

done

The same is done by Vincent on ThunderX2 and no issues were seen.

Hotplug worked fine for me as well on both platforms I tested (Juno R2 and ThunderX2).

Thanks,
Ionela.

Quoted 97 lines · Ionela Voinescu

Hi guys,

Quoted 7 lines · Viresh Kumar

Hello,

Changes since V2:

- We don't need start_cpu() and stop_cpu() callbacks anymore, we can make it work using policy ->init() and exit() alone.

Quoted 7 lines

- Two new cleanup patches 1/4 and 2/4.

- Improved commit log of 3/4.

- Dropped WARN_ON(local_freq_scale > 1024), since this can occur on counter's overlap (seen with Vincent's setup).

If you happen to have the data around, I would like to know more about your observations on ThunderX2.

I tried ThunderX2 as well, with the following observations:

Booting with userspace governor and all CPUs online, the CPPC frequency scale factor was all over the place (even much larger than 1024).

My initial assumptions:
- Counters do not behave properly in light of SMT
- Firmware does not do a good job to keep the reference and core counters monotonic: save and restore at core off.

So I offlined all CPUs with the exception of 0, 32, 64, 96 - threads of a single core (part of policy0). With this all works very well:

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1056000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1863.095370] CPU96: cppc scale: 697.
[ 1863.175370] CPU0: cppc scale: 492.
[ 1863.215367] CPU64: cppc scale: 492.
[ 1863.235366] CPU96: cppc scale: 492.
[ 1863.485368] CPU32: cppc scale: 492.

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1936000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1891.395363] CPU96: cppc scale: 558.
[ 1891.415362] CPU0: cppc scale: 595.
[ 1891.435362] CPU32: cppc scale: 615.
[ 1891.465363] CPU96: cppc scale: 635.
[ 1891.495361] CPU0: cppc scale: 673.
[ 1891.515360] CPU32: cppc scale: 703.
[ 1891.545360] CPU96: cppc scale: 738.
[ 1891.575360] CPU0: cppc scale: 779.
[ 1891.605360] CPU96: cppc scale: 829.
[ 1891.635360] CPU0: cppc scale: 879.

root@target:/sys/devices/system/cpu/cpufreq/policy0#

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 2200000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1896.585363] CPU32: cppc scale: 1004.
[ 1896.675359] CPU64: cppc scale: 973.
[ 1896.715359] CPU0: cppc scale: 1024.

I'm doing a rate limited printk only for increase/decrease values over 64 in the scale factor value.

This showed me that SMT is handled properly.

Then, as soon as I start onlining CPUs 1, 33, 65, 97, the scale factor stops being even close to correct, for example:

[238394.770328] CPU96: cppc scale: 22328.
[238395.628846] CPU96: cppc scale: 245.
[238516.087115] CPU96: cppc scale: 930.
[238523.385009] CPU96: cppc scale: 245.
[238538.767473] CPU96: cppc scale: 936.
[238538.867546] CPU96: cppc scale: 245.
[238599.367932] CPU97: cppc scale: 2728.
[238599.859865] CPU97: cppc scale: 452.
[238647.786284] CPU96: cppc scale: 1438.
[238669.604684] CPU96: cppc scale: 27306.
[238676.805049] CPU96: cppc scale: 245.
[238737.642902] CPU97: cppc scale: 2035.
[238737.664995] CPU97: cppc scale: 452.
[238788.066193] CPU96: cppc scale: 2749.
[238788.110192] CPU96: cppc scale: 245.
[238817.231659] CPU96: cppc scale: 2698.
[238818.083687] CPU96: cppc scale: 245.
[238845.466850] CPU97: cppc scale: 2990.
[238847.477805] CPU97: cppc scale: 452.
[238936.984107] CPU97: cppc scale: 1590.
[238937.029079] CPU97: cppc scale: 452.
[238979.052464] CPU97: cppc scale: 911.
[238980.900668] CPU97: cppc scale: 452.
[239149.587889] CPU96: cppc scale: 803.
[239151.085516] CPU96: cppc scale: 245.

Quoted 9 lines

[239303.871373] CPU64: cppc scale: 956.
[239303.906837] CPU64: cppc scale: 245.
[239308.666786] CPU96: cppc scale: 821.
[239319.440634] CPU96: cppc scale: 245.
[239389.978395] CPU97: cppc scale: 4229.
[239391.969562] CPU97: cppc scale: 452.
[239415.894738] CPU96: cppc scale: 630.
[239417.875326] CPU96: cppc scale: 245.

With the counter being 32bits and the freq scaling being update at tick, you can easily get a overflow on it in idle system. I can easily imagine that when you unplug CPUs there is enough activity on the CPU to update it regularly whereas with all CPUs, the idle time is longer that the counter overflow

Quoted 11 lines

The counter values shown by feedback_ctrs do not seem monotonic even when only core 0 threads are online.

ref:2812420736 del:166051103 ref:3683620736 del:641578595 ref:1049653440 del:1548202980 ref:2099053440 del:2120997459 ref:3185853440 del:2714205997 ref:712486144 del:3708490753 ref:3658438336 del:3401357212 ref:1570998080 del:2279728438

Quoted 27 lines

For now I was just wondering if you have seen the same and whether you have an opinion on this.

Quoted 13 lines

This is tested on my Hikey platform (without the actual read/write to performance counters), with this script for over an hour:

while true; do

    for i in `seq 1 7`;
    do
        echo 0 > /sys/devices/system/cpu/cpu$i/online;
    done;

    for i in `seq 1 7`;
    do
        echo 1 > /sys/devices/system/cpu/cpu$i/online;
    done;

Quoted 4 lines

done

The same is done by Vincent on ThunderX2 and no issues were seen.

Hotplug worked fine for me as well on both platforms I tested (Juno R2 and ThunderX2).

Thanks,
Ionela.

Quoted 97 lines · Ionela Voinescu

Hi guys,

Quoted 7 lines · Viresh Kumar

Hello,

Changes since V2:

- We don't need start_cpu() and stop_cpu() callbacks anymore, we can make it work using policy ->init() and exit() alone.

Quoted 7 lines

- Two new cleanup patches 1/4 and 2/4.

- Improved commit log of 3/4.

- Dropped WARN_ON(local_freq_scale > 1024), since this can occur on counter's overlap (seen with Vincent's setup).

If you happen to have the data around, I would like to know more about your observations on ThunderX2.

I tried ThunderX2 as well, with the following observations:

Booting with userspace governor and all CPUs online, the CPPC frequency scale factor was all over the place (even much larger than 1024).

My initial assumptions:
- Counters do not behave properly in light of SMT
- Firmware does not do a good job to keep the reference and core counters monotonic: save and restore at core off.

So I offlined all CPUs with the exception of 0, 32, 64, 96 - threads of a single core (part of policy0). With this all works very well:

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1056000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1863.095370] CPU96: cppc scale: 697.
[ 1863.175370] CPU0: cppc scale: 492.
[ 1863.215367] CPU64: cppc scale: 492.
[ 1863.235366] CPU96: cppc scale: 492.
[ 1863.485368] CPU32: cppc scale: 492.

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1936000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1891.395363] CPU96: cppc scale: 558.
[ 1891.415362] CPU0: cppc scale: 595.
[ 1891.435362] CPU32: cppc scale: 615.
[ 1891.465363] CPU96: cppc scale: 635.
[ 1891.495361] CPU0: cppc scale: 673.
[ 1891.515360] CPU32: cppc scale: 703.
[ 1891.545360] CPU96: cppc scale: 738.
[ 1891.575360] CPU0: cppc scale: 779.
[ 1891.605360] CPU96: cppc scale: 829.
[ 1891.635360] CPU0: cppc scale: 879.

root@target:/sys/devices/system/cpu/cpufreq/policy0#

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 2200000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1896.585363] CPU32: cppc scale: 1004.
[ 1896.675359] CPU64: cppc scale: 973.
[ 1896.715359] CPU0: cppc scale: 1024.

I'm doing a rate limited printk only for increase/decrease values over 64 in the scale factor value.

This showed me that SMT is handled properly.

Then, as soon as I start onlining CPUs 1, 33, 65, 97, the scale factor stops being even close to correct, for example:

[238394.770328] CPU96: cppc scale: 22328.
[238395.628846] CPU96: cppc scale: 245.
[238516.087115] CPU96: cppc scale: 930.
[238523.385009] CPU96: cppc scale: 245.
[238538.767473] CPU96: cppc scale: 936.
[238538.867546] CPU96: cppc scale: 245.
[238599.367932] CPU97: cppc scale: 2728.
[238599.859865] CPU97: cppc scale: 452.
[238647.786284] CPU96: cppc scale: 1438.
[238669.604684] CPU96: cppc scale: 27306.
[238676.805049] CPU96: cppc scale: 245.
[238737.642902] CPU97: cppc scale: 2035.
[238737.664995] CPU97: cppc scale: 452.
[238788.066193] CPU96: cppc scale: 2749.
[238788.110192] CPU96: cppc scale: 245.
[238817.231659] CPU96: cppc scale: 2698.
[238818.083687] CPU96: cppc scale: 245.
[238845.466850] CPU97: cppc scale: 2990.
[238847.477805] CPU97: cppc scale: 452.
[238936.984107] CPU97: cppc scale: 1590.
[238937.029079] CPU97: cppc scale: 452.
[238979.052464] CPU97: cppc scale: 911.
[238980.900668] CPU97: cppc scale: 452.
[239149.587889] CPU96: cppc scale: 803.
[239151.085516] CPU96: cppc scale: 245.

Quoted 9 lines

[239303.871373] CPU64: cppc scale: 956.
[239303.906837] CPU64: cppc scale: 245.
[239308.666786] CPU96: cppc scale: 821.
[239319.440634] CPU96: cppc scale: 245.
[239389.978395] CPU97: cppc scale: 4229.
[239391.969562] CPU97: cppc scale: 452.
[239415.894738] CPU96: cppc scale: 630.
[239417.875326] CPU96: cppc scale: 245.

With the counter being 32bits and the freq scaling being update at tick, you can easily get a overflow on it in idle system. I can easily imagine that when you unplug CPUs there is enough activity on the CPU to update it regularly whereas with all CPUs, the idle time is longer that the counter overflow

There are 32bits and the overflow need to be handled by cppc_cpufreq driver

Quoted 38 lines

The counter values shown by feedback_ctrs do not seem monotonic even when only core 0 threads are online.

ref:2812420736 del:166051103 ref:3683620736 del:641578595 ref:1049653440 del:1548202980 ref:2099053440 del:2120997459 ref:3185853440 del:2714205997 ref:712486144 del:3708490753 ref:3658438336 del:3401357212 ref:1570998080 del:2279728438

For now I was just wondering if you have seen the same and whether you have an opinion on this.

Quoted 13 lines

This is tested on my Hikey platform (without the actual read/write to performance counters), with this script for over an hour:

while true; do

    for i in `seq 1 7`;
    do
        echo 0 > /sys/devices/system/cpu/cpu$i/online;
    done;

    for i in `seq 1 7`;
    do
        echo 1 > /sys/devices/system/cpu/cpu$i/online;
    done;

Quoted 4 lines

done

The same is done by Vincent on ThunderX2 and no issues were seen.

Hotplug worked fine for me as well on both platforms I tested (Juno R2 and ThunderX2).

Thanks,
Ionela.

Quoted 108 lines · Vincent Guittot

Quoted 106 lines · Ionela Voinescu

Hi guys,

Quoted 7 lines · Viresh Kumar

Hello,

Changes since V2:

- We don't need start_cpu() and stop_cpu() callbacks anymore, we can make it work using policy ->init() and exit() alone.

Quoted 7 lines

- Two new cleanup patches 1/4 and 2/4.

- Improved commit log of 3/4.

- Dropped WARN_ON(local_freq_scale > 1024), since this can occur on counter's overlap (seen with Vincent's setup).

If you happen to have the data around, I would like to know more about your observations on ThunderX2.

I tried ThunderX2 as well, with the following observations:

Booting with userspace governor and all CPUs online, the CPPC frequency scale factor was all over the place (even much larger than 1024).

My initial assumptions:
- Counters do not behave properly in light of SMT
- Firmware does not do a good job to keep the reference and core counters monotonic: save and restore at core off.

So I offlined all CPUs with the exception of 0, 32, 64, 96 - threads of a single core (part of policy0). With this all works very well:

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1056000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1863.095370] CPU96: cppc scale: 697.
[ 1863.175370] CPU0: cppc scale: 492.
[ 1863.215367] CPU64: cppc scale: 492.
[ 1863.235366] CPU96: cppc scale: 492.
[ 1863.485368] CPU32: cppc scale: 492.

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1936000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1891.395363] CPU96: cppc scale: 558.
[ 1891.415362] CPU0: cppc scale: 595.
[ 1891.435362] CPU32: cppc scale: 615.
[ 1891.465363] CPU96: cppc scale: 635.
[ 1891.495361] CPU0: cppc scale: 673.
[ 1891.515360] CPU32: cppc scale: 703.
[ 1891.545360] CPU96: cppc scale: 738.
[ 1891.575360] CPU0: cppc scale: 779.
[ 1891.605360] CPU96: cppc scale: 829.
[ 1891.635360] CPU0: cppc scale: 879.

root@target:/sys/devices/system/cpu/cpufreq/policy0#

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 2200000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1896.585363] CPU32: cppc scale: 1004.
[ 1896.675359] CPU64: cppc scale: 973.
[ 1896.715359] CPU0: cppc scale: 1024.

I'm doing a rate limited printk only for increase/decrease values over 64 in the scale factor value.

This showed me that SMT is handled properly.

Then, as soon as I start onlining CPUs 1, 33, 65, 97, the scale factor stops being even close to correct, for example:

[238394.770328] CPU96: cppc scale: 22328.
[238395.628846] CPU96: cppc scale: 245.
[238516.087115] CPU96: cppc scale: 930.
[238523.385009] CPU96: cppc scale: 245.
[238538.767473] CPU96: cppc scale: 936.
[238538.867546] CPU96: cppc scale: 245.
[238599.367932] CPU97: cppc scale: 2728.
[238599.859865] CPU97: cppc scale: 452.
[238647.786284] CPU96: cppc scale: 1438.
[238669.604684] CPU96: cppc scale: 27306.
[238676.805049] CPU96: cppc scale: 245.
[238737.642902] CPU97: cppc scale: 2035.
[238737.664995] CPU97: cppc scale: 452.
[238788.066193] CPU96: cppc scale: 2749.
[238788.110192] CPU96: cppc scale: 245.
[238817.231659] CPU96: cppc scale: 2698.
[238818.083687] CPU96: cppc scale: 245.
[238845.466850] CPU97: cppc scale: 2990.
[238847.477805] CPU97: cppc scale: 452.
[238936.984107] CPU97: cppc scale: 1590.
[238937.029079] CPU97: cppc scale: 452.
[238979.052464] CPU97: cppc scale: 911.
[238980.900668] CPU97: cppc scale: 452.
[239149.587889] CPU96: cppc scale: 803.
[239151.085516] CPU96: cppc scale: 245.
[239303.871373] CPU64: cppc scale: 956.
[239303.906837] CPU64: cppc scale: 245.
[239308.666786] CPU96: cppc scale: 821.
[239319.440634] CPU96: cppc scale: 245.
[239389.978395] CPU97: cppc scale: 4229.
[239391.969562] CPU97: cppc scale: 452.
[239415.894738] CPU96: cppc scale: 630.
[239417.875326] CPU96: cppc scale: 245.

Quoted 5 lines

With the counter being 32bits and the freq scaling being update at tick, you can easily get a overflow on it in idle system. I can easily imagine that when you unplug CPUs there is enough activity on the CPU to update it regularly whereas with all CPUs, the idle time is longer that the counter overflow

Quoted 1 line

Thanks! Yes, given the high wraparound time I thought they were 64 bit. All variables in software are 64 bit, but looking at bit width in the _CPC entries, the platform counters are 32 bit counters.

Quoted 2 lines

There are 32bits and the overflow need to be handled by cppc_cpufreq driver

I'm wondering if this would be best handled in the function that reads the counters or in the cppc_cpufreq driver that uses them. Probably the latter, as you say, as the read function should only return the raw values, but it does complicate things.

Thanks,
Ionela.

Quoted 38 lines

Quoted 38 lines

The counter values shown by feedback_ctrs do not seem monotonic even when only core 0 threads are online.

ref:2812420736 del:166051103 ref:3683620736 del:641578595 ref:1049653440 del:1548202980 ref:2099053440 del:2120997459 ref:3185853440 del:2714205997 ref:712486144 del:3708490753 ref:3658438336 del:3401357212 ref:1570998080 del:2279728438

For now I was just wondering if you have seen the same and whether you have an opinion on this.

Quoted 13 lines

This is tested on my Hikey platform (without the actual read/write to performance counters), with this script for over an hour:

while true; do

    for i in `seq 1 7`;
    do
        echo 0 > /sys/devices/system/cpu/cpu$i/online;
    done;

    for i in `seq 1 7`;
    do
        echo 1 > /sys/devices/system/cpu/cpu$i/online;
    done;

Quoted 4 lines

done

The same is done by Vincent on ThunderX2 and no issues were seen.

Hotplug worked fine for me as well on both platforms I tested (Juno R2 and ThunderX2).

Thanks,
Ionela.

Quoted 109 lines · Ionela Voinescu

Quoted 108 lines · Vincent Guittot

Quoted 106 lines · Ionela Voinescu

Hi guys,

Quoted 7 lines · Viresh Kumar

Hello,

Changes since V2:

- We don't need start_cpu() and stop_cpu() callbacks anymore, we can make it work using policy ->init() and exit() alone.

Quoted 7 lines

- Two new cleanup patches 1/4 and 2/4.

- Improved commit log of 3/4.

- Dropped WARN_ON(local_freq_scale > 1024), since this can occur on counter's overlap (seen with Vincent's setup).

If you happen to have the data around, I would like to know more about your observations on ThunderX2.

I tried ThunderX2 as well, with the following observations:

Booting with userspace governor and all CPUs online, the CPPC frequency scale factor was all over the place (even much larger than 1024).

My initial assumptions:
- Counters do not behave properly in light of SMT
- Firmware does not do a good job to keep the reference and core counters monotonic: save and restore at core off.

So I offlined all CPUs with the exception of 0, 32, 64, 96 - threads of a single core (part of policy0). With this all works very well:

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1056000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1863.095370] CPU96: cppc scale: 697.
[ 1863.175370] CPU0: cppc scale: 492.
[ 1863.215367] CPU64: cppc scale: 492.
[ 1863.235366] CPU96: cppc scale: 492.
[ 1863.485368] CPU32: cppc scale: 492.

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1936000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1891.395363] CPU96: cppc scale: 558.
[ 1891.415362] CPU0: cppc scale: 595.
[ 1891.435362] CPU32: cppc scale: 615.
[ 1891.465363] CPU96: cppc scale: 635.
[ 1891.495361] CPU0: cppc scale: 673.
[ 1891.515360] CPU32: cppc scale: 703.
[ 1891.545360] CPU96: cppc scale: 738.
[ 1891.575360] CPU0: cppc scale: 779.
[ 1891.605360] CPU96: cppc scale: 829.
[ 1891.635360] CPU0: cppc scale: 879.

root@target:/sys/devices/system/cpu/cpufreq/policy0#

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 2200000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1896.585363] CPU32: cppc scale: 1004.
[ 1896.675359] CPU64: cppc scale: 973.
[ 1896.715359] CPU0: cppc scale: 1024.

I'm doing a rate limited printk only for increase/decrease values over 64 in the scale factor value.

This showed me that SMT is handled properly.

Then, as soon as I start onlining CPUs 1, 33, 65, 97, the scale factor stops being even close to correct, for example:

[238394.770328] CPU96: cppc scale: 22328.
[238395.628846] CPU96: cppc scale: 245.
[238516.087115] CPU96: cppc scale: 930.
[238523.385009] CPU96: cppc scale: 245.
[238538.767473] CPU96: cppc scale: 936.
[238538.867546] CPU96: cppc scale: 245.
[238599.367932] CPU97: cppc scale: 2728.
[238599.859865] CPU97: cppc scale: 452.
[238647.786284] CPU96: cppc scale: 1438.
[238669.604684] CPU96: cppc scale: 27306.
[238676.805049] CPU96: cppc scale: 245.
[238737.642902] CPU97: cppc scale: 2035.
[238737.664995] CPU97: cppc scale: 452.
[238788.066193] CPU96: cppc scale: 2749.
[238788.110192] CPU96: cppc scale: 245.
[238817.231659] CPU96: cppc scale: 2698.
[238818.083687] CPU96: cppc scale: 245.
[238845.466850] CPU97: cppc scale: 2990.
[238847.477805] CPU97: cppc scale: 452.
[238936.984107] CPU97: cppc scale: 1590.
[238937.029079] CPU97: cppc scale: 452.
[238979.052464] CPU97: cppc scale: 911.
[238980.900668] CPU97: cppc scale: 452.
[239149.587889] CPU96: cppc scale: 803.
[239151.085516] CPU96: cppc scale: 245.
[239303.871373] CPU64: cppc scale: 956.
[239303.906837] CPU64: cppc scale: 245.
[239308.666786] CPU96: cppc scale: 821.
[239319.440634] CPU96: cppc scale: 245.
[239389.978395] CPU97: cppc scale: 4229.
[239391.969562] CPU97: cppc scale: 452.
[239415.894738] CPU96: cppc scale: 630.
[239417.875326] CPU96: cppc scale: 245.

Quoted 5 lines

Quoted 5 lines

With the counter being 32bits and the freq scaling being update at tick, you can easily get a overflow on it in idle system. I can easily imagine that when you unplug CPUs there is enough activity on the CPU to update it regularly whereas with all CPUs, the idle time is longer that the counter overflow

For sane counters, how long the CPU is idle should not matter (please see below my definition of sane counters).

Quoted 6 lines

Quoted 1 line

Thanks! Yes, given the high wraparound time I thought they were 64 bit. All variables in software are 64 bit, but looking at bit width in the _CPC entries, the platform counters are 32 bit counters.

I've looked a bit more over the code, and considering this particular system (32 bit counters, maximum frequency of CPUs = 2.2GHz), I believe the wraparound is considered, and this should not cause these strange values in the scale factor.

I consider the counters sane if both stop during idle - either they stop when CPU is clock gated, or some firmware does save/restore at core off. Therefore, in all idle cases they seem to have stopped, from the point of view of the OS. The ACPI spec mentions that both count "any time the processor is active".

After the cores return from idle, the counters will wraparound at a minimum of 1.95s. So with a tick every 4ms at most 1 wraparound would have happened which allows the getDelta() function in cppc_cpufreq driver to get the proper difference in values.

Let me know what you think.

Thanks,
Ionela.

Quoted 51 lines

Quoted 2 lines

There are 32bits and the overflow need to be handled by cppc_cpufreq driver

I'm wondering if this would be best handled in the function that reads the counters or in the cppc_cpufreq driver that uses them. Probably the latter, as you say, as the read function should only return the raw values, but it does complicate things.

Thanks,
Ionela.

Quoted 38 lines

Quoted 38 lines

The counter values shown by feedback_ctrs do not seem monotonic even when only core 0 threads are online.

ref:2812420736 del:166051103 ref:3683620736 del:641578595 ref:1049653440 del:1548202980 ref:2099053440 del:2120997459 ref:3185853440 del:2714205997 ref:712486144 del:3708490753 ref:3658438336 del:3401357212 ref:1570998080 del:2279728438

For now I was just wondering if you have seen the same and whether you have an opinion on this.

Quoted 13 lines

This is tested on my Hikey platform (without the actual read/write to performance counters), with this script for over an hour:

while true; do

    for i in `seq 1 7`;
    do
        echo 0 > /sys/devices/system/cpu/cpu$i/online;
    done;

    for i in `seq 1 7`;
    do
        echo 1 > /sys/devices/system/cpu/cpu$i/online;
    done;

Quoted 4 lines

done

The same is done by Vincent on ThunderX2 and no issues were seen.

Hotplug worked fine for me as well on both platforms I tested (Juno R2 and ThunderX2).

Thanks,
Ionela.

If you happen to have the data around, I would like to know more about your observations on ThunderX2.

I tried ThunderX2 as well, with the following observations:

Booting with userspace governor and all CPUs online, the CPPC frequency scale factor was all over the place (even much larger than 1024).

My initial assumptions:
- Counters do not behave properly in light of SMT
- Firmware does not do a good job to keep the reference and core counters monotonic: save and restore at core off.

So I offlined all CPUs with the exception of 0, 32, 64, 96 - threads of a single core (part of policy0). With this all works very well:

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1056000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1863.095370] CPU96: cppc scale: 697.
[ 1863.175370] CPU0: cppc scale: 492.
[ 1863.215367] CPU64: cppc scale: 492.
[ 1863.235366] CPU96: cppc scale: 492.
[ 1863.485368] CPU32: cppc scale: 492.

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1936000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1891.395363] CPU96: cppc scale: 558.
[ 1891.415362] CPU0: cppc scale: 595.
[ 1891.435362] CPU32: cppc scale: 615.
[ 1891.465363] CPU96: cppc scale: 635.
[ 1891.495361] CPU0: cppc scale: 673.
[ 1891.515360] CPU32: cppc scale: 703.
[ 1891.545360] CPU96: cppc scale: 738.
[ 1891.575360] CPU0: cppc scale: 779.
[ 1891.605360] CPU96: cppc scale: 829.
[ 1891.635360] CPU0: cppc scale: 879.

root@target:/sys/devices/system/cpu/cpufreq/policy0#

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 2200000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1896.585363] CPU32: cppc scale: 1004.
[ 1896.675359] CPU64: cppc scale: 973.
[ 1896.715359] CPU0: cppc scale: 1024.

I'm doing a rate limited printk only for increase/decrease values over 64 in the scale factor value.

This showed me that SMT is handled properly.

Then, as soon as I start onlining CPUs 1, 33, 65, 97, the scale factor stops being even close to correct, for example:

[238394.770328] CPU96: cppc scale: 22328.
[238395.628846] CPU96: cppc scale: 245.
[238516.087115] CPU96: cppc scale: 930.
[238523.385009] CPU96: cppc scale: 245.
[238538.767473] CPU96: cppc scale: 936.
[238538.867546] CPU96: cppc scale: 245.
[238599.367932] CPU97: cppc scale: 2728.
[238599.859865] CPU97: cppc scale: 452.
[238647.786284] CPU96: cppc scale: 1438.
[238669.604684] CPU96: cppc scale: 27306.
[238676.805049] CPU96: cppc scale: 245.
[238737.642902] CPU97: cppc scale: 2035.
[238737.664995] CPU97: cppc scale: 452.
[238788.066193] CPU96: cppc scale: 2749.
[238788.110192] CPU96: cppc scale: 245.
[238817.231659] CPU96: cppc scale: 2698.
[238818.083687] CPU96: cppc scale: 245.
[238845.466850] CPU97: cppc scale: 2990.
[238847.477805] CPU97: cppc scale: 452.
[238936.984107] CPU97: cppc scale: 1590.
[238937.029079] CPU97: cppc scale: 452.
[238979.052464] CPU97: cppc scale: 911.
[238980.900668] CPU97: cppc scale: 452.
[239149.587889] CPU96: cppc scale: 803.
[239151.085516] CPU96: cppc scale: 245.
[239303.871373] CPU64: cppc scale: 956.
[239303.906837] CPU64: cppc scale: 245.
[239308.666786] CPU96: cppc scale: 821.
[239319.440634] CPU96: cppc scale: 245.
[239389.978395] CPU97: cppc scale: 4229.
[239391.969562] CPU97: cppc scale: 452.
[239415.894738] CPU96: cppc scale: 630.
[239417.875326] CPU96: cppc scale: 245.

The counter values shown by feedback_ctrs do not seem monotonic even when only core 0 threads are online.

ref:2812420736 del:166051103 ref:3683620736 del:641578595 ref:1049653440 del:1548202980 ref:2099053440 del:2120997459 ref:3185853440 del:2714205997 ref:712486144 del:3708490753 ref:3658438336 del:3401357212 ref:1570998080 del:2279728438

For now I was just wondering if you have seen the same and whether you have an opinion on this.

Quoted 9 lines · Ionela Voinescu

If you happen to have the data around, I would like to know more about your observations on ThunderX2.

I tried ThunderX2 as well, with the following observations:

Booting with userspace governor and all CPUs online, the CPPC frequency scale factor was all over the place (even much larger than 1024).

Quoted 7 lines

My initial assumptions:
- Counters do not behave properly in light of SMT
- Firmware does not do a good job to keep the reference and core counters monotonic: save and restore at core off.

So I offlined all CPUs with the exception of 0, 32, 64, 96 - threads of a single core (part of policy0). With this all works very well:

Interesting.

Quoted 82 lines

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1056000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1863.095370] CPU96: cppc scale: 697.
[ 1863.175370] CPU0: cppc scale: 492.
[ 1863.215367] CPU64: cppc scale: 492.
[ 1863.235366] CPU96: cppc scale: 492.
[ 1863.485368] CPU32: cppc scale: 492.

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 1936000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1891.395363] CPU96: cppc scale: 558.
[ 1891.415362] CPU0: cppc scale: 595.
[ 1891.435362] CPU32: cppc scale: 615.
[ 1891.465363] CPU96: cppc scale: 635.
[ 1891.495361] CPU0: cppc scale: 673.
[ 1891.515360] CPU32: cppc scale: 703.
[ 1891.545360] CPU96: cppc scale: 738.
[ 1891.575360] CPU0: cppc scale: 779.
[ 1891.605360] CPU96: cppc scale: 829.
[ 1891.635360] CPU0: cppc scale: 879.

root@target:/sys/devices/system/cpu/cpufreq/policy0#

root@target:/sys/devices/system/cpu/cpufreq/policy0# echo 2200000 > scaling_setspeed
root@target:/sys/devices/system/cpu/cpufreq/policy0#
[ 1896.585363] CPU32: cppc scale: 1004.
[ 1896.675359] CPU64: cppc scale: 973.
[ 1896.715359] CPU0: cppc scale: 1024.

I'm doing a rate limited printk only for increase/decrease values over 64 in the scale factor value.

This showed me that SMT is handled properly.

Then, as soon as I start onlining CPUs 1, 33, 65, 97, the scale factor stops being even close to correct, for example:

[238394.770328] CPU96: cppc scale: 22328.
[238395.628846] CPU96: cppc scale: 245.
[238516.087115] CPU96: cppc scale: 930.
[238523.385009] CPU96: cppc scale: 245.
[238538.767473] CPU96: cppc scale: 936.
[238538.867546] CPU96: cppc scale: 245.
[238599.367932] CPU97: cppc scale: 2728.
[238599.859865] CPU97: cppc scale: 452.
[238647.786284] CPU96: cppc scale: 1438.
[238669.604684] CPU96: cppc scale: 27306.
[238676.805049] CPU96: cppc scale: 245.
[238737.642902] CPU97: cppc scale: 2035.
[238737.664995] CPU97: cppc scale: 452.
[238788.066193] CPU96: cppc scale: 2749.
[238788.110192] CPU96: cppc scale: 245.
[238817.231659] CPU96: cppc scale: 2698.
[238818.083687] CPU96: cppc scale: 245.
[238845.466850] CPU97: cppc scale: 2990.
[238847.477805] CPU97: cppc scale: 452.
[238936.984107] CPU97: cppc scale: 1590.
[238937.029079] CPU97: cppc scale: 452.
[238979.052464] CPU97: cppc scale: 911.
[238980.900668] CPU97: cppc scale: 452.
[239149.587889] CPU96: cppc scale: 803.
[239151.085516] CPU96: cppc scale: 245.
[239303.871373] CPU64: cppc scale: 956.
[239303.906837] CPU64: cppc scale: 245.
[239308.666786] CPU96: cppc scale: 821.
[239319.440634] CPU96: cppc scale: 245.
[239389.978395] CPU97: cppc scale: 4229.
[239391.969562] CPU97: cppc scale: 452.
[239415.894738] CPU96: cppc scale: 630.
[239417.875326] CPU96: cppc scale: 245.

The counter values shown by feedback_ctrs do not seem monotonic even when only core 0 threads are online.

ref:2812420736 del:166051103 ref:3683620736 del:641578595 ref:1049653440 del:1548202980 ref:2099053440 del:2120997459 ref:3185853440 del:2714205997 ref:712486144 del:3708490753 ref:3658438336 del:3401357212 ref:1570998080 del:2279728438

Quoted 2 lines

For now I was just wondering if you have seen the same and whether you have an opinion on this.

I think we also saw numbers like this, which didn't explain a lot on ThunderX2. We thought they may be due to rounding issues, but the offlining stuff adds an interesting factor to that.

Signature · 1 line

viresh