Patchwork [v2,2/6] powernv:cpufreq: Create a powernv_cpu_to_core_mask() helper.

login
register
mail settings
Submitter Gautham R. Shenoy
Date March 10, 2014, 11:10 a.m.
Message ID <1394449861-8688-3-git-send-email-ego@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/328534/
State Superseded
Headers show

Comments

Gautham R. Shenoy - March 10, 2014, 11:10 a.m.
From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>

Create a helper method that computes the cpumask corresponding to the
thread-siblings of a cpu. Use this for initializing the policy->cpus
mask for a given cpu.

(Original code written by Srivatsa S. Bhat. Gautham moved this to a
helper function!)

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 drivers/cpufreq/powernv-cpufreq.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
Benjamin Herrenschmidt - March 18, 2014, 11:37 p.m.
On Mon, 2014-03-10 at 16:40 +0530, Gautham R. Shenoy wrote:
> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> 
> Create a helper method that computes the cpumask corresponding to the
> thread-siblings of a cpu. Use this for initializing the policy->cpus
> mask for a given cpu.
> 
> (Original code written by Srivatsa S. Bhat. Gautham moved this to a
> helper function!)

How does that differ from the existing entry in the sibling map  which
you can obtain with the helper cpu_sibling_mask() ? (You probably want
to *copy* the mask of course but I don't see the need of re-creating
one from scratch).

Also, this should have been CCed to the cpufreq mailing list and maybe
the relevant maintainer too.

Cheers,
Ben.

> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index ab1551f..4cad727 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -115,6 +115,23 @@ static struct freq_attr *powernv_cpu_freq_attr[] = {
>  
>  /* Helper routines */
>  
> +/**
> + * Sets the bits corresponding to the thread-siblings of cpu in its core
> + * in 'cpus'.
> + */
> +static void powernv_cpu_to_core_mask(unsigned int cpu, cpumask_var_t cpus)
> +{
> +	int base, i;
> +
> +	base = cpu_first_thread_sibling(cpu);
> +
> +	for (i = 0; i < threads_per_core; i++) {
> +		cpumask_set_cpu(base + i, cpus);
> +	}
> +
> +	return;
> +}
> +
>  /* Access helpers to power mgt SPR */
>  
>  static inline unsigned long get_pmspr(unsigned long sprn)
> @@ -180,13 +197,8 @@ static int powernv_set_freq(cpumask_var_t cpus, unsigned int new_index)
>  
>  static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  {
> -	int base, i;
> -
>  #ifdef CONFIG_SMP
> -	base = cpu_first_thread_sibling(policy->cpu);
> -
> -	for (i = 0; i < threads_per_core; i++)
> -		cpumask_set_cpu(base + i, policy->cpus);
> +	powernv_cpu_to_core_mask(policy->cpu, policy->cpus);
>  #endif
>  	policy->cpuinfo.transition_latency = 25000;
>
Srivatsa S. Bhat - March 19, 2014, 6:35 a.m.
On 03/19/2014 05:07 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2014-03-10 at 16:40 +0530, Gautham R. Shenoy wrote:
>> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>>
>> Create a helper method that computes the cpumask corresponding to the
>> thread-siblings of a cpu. Use this for initializing the policy->cpus
>> mask for a given cpu.
>>
>> (Original code written by Srivatsa S. Bhat. Gautham moved this to a
>> helper function!)
> 
> How does that differ from the existing entry in the sibling map  which
> you can obtain with the helper cpu_sibling_mask() ? (You probably want
> to *copy* the mask of course but I don't see the need of re-creating
> one from scratch).
>

The intent here was to have a sibling mask that is invariant of CPU
hotplug. cpu_sibling_mask is updated on every hotplug operation, and this
would break cpufreq, since policy->cpus has to be hotplug invariant.

This should have been noted in the changelog of patch 1 as well as this
patch. (The earlier (internal) versions of this patchset had the
description in the changelogs, but somehow it got dropped accidentally).
I'll work with Gautham and ensure that we have this important info
included in the changelog. Thanks for pointing it out!

Regards,
Srivatsa S. Bhat

 
> Also, this should have been CCed to the cpufreq mailing list and maybe
> the relevant maintainer too.
> 
> Cheers,
> Ben.
> 
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
>> ---
>>  drivers/cpufreq/powernv-cpufreq.c | 24 ++++++++++++++++++------
>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index ab1551f..4cad727 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -115,6 +115,23 @@ static struct freq_attr *powernv_cpu_freq_attr[] = {
>>  
>>  /* Helper routines */
>>  
>> +/**
>> + * Sets the bits corresponding to the thread-siblings of cpu in its core
>> + * in 'cpus'.
>> + */
>> +static void powernv_cpu_to_core_mask(unsigned int cpu, cpumask_var_t cpus)
>> +{
>> +	int base, i;
>> +
>> +	base = cpu_first_thread_sibling(cpu);
>> +
>> +	for (i = 0; i < threads_per_core; i++) {
>> +		cpumask_set_cpu(base + i, cpus);
>> +	}
>> +
>> +	return;
>> +}
>> +
>>  /* Access helpers to power mgt SPR */
>>  
>>  static inline unsigned long get_pmspr(unsigned long sprn)
>> @@ -180,13 +197,8 @@ static int powernv_set_freq(cpumask_var_t cpus, unsigned int new_index)
>>  
>>  static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>  {
>> -	int base, i;
>> -
>>  #ifdef CONFIG_SMP
>> -	base = cpu_first_thread_sibling(policy->cpu);
>> -
>> -	for (i = 0; i < threads_per_core; i++)
>> -		cpumask_set_cpu(base + i, policy->cpus);
>> +	powernv_cpu_to_core_mask(policy->cpu, policy->cpus);
>>  #endif
>>  	policy->cpuinfo.transition_latency = 25000;
>>  
> 
>
Gautham R. Shenoy - March 19, 2014, 11:02 a.m.
On Wed, Mar 19, 2014 at 12:05:20PM +0530, Srivatsa S. Bhat wrote:
> On 03/19/2014 05:07 AM, Benjamin Herrenschmidt wrote:
> > On Mon, 2014-03-10 at 16:40 +0530, Gautham R. Shenoy wrote:
> >> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> >>
> >> Create a helper method that computes the cpumask corresponding to the
> >> thread-siblings of a cpu. Use this for initializing the policy->cpus
> >> mask for a given cpu.
> >>
> >> (Original code written by Srivatsa S. Bhat. Gautham moved this to a
> >> helper function!)
> > 
> > How does that differ from the existing entry in the sibling map  which
> > you can obtain with the helper cpu_sibling_mask() ? (You probably want
> > to *copy* the mask of course but I don't see the need of re-creating
> > one from scratch).
> >
> 
> The intent here was to have a sibling mask that is invariant of CPU
> hotplug. cpu_sibling_mask is updated on every hotplug operation, and this
> would break cpufreq, since policy->cpus has to be hotplug invariant.
> 
> This should have been noted in the changelog of patch 1 as well as this
> patch. (The earlier (internal) versions of this patchset had the
> description in the changelogs, but somehow it got dropped accidentally).
> I'll work with Gautham and ensure that we have this important info
> included in the changelog. Thanks for pointing it out!

I reused that part of the code because I was unaware of
cpu_sibling_mask()!

For implementing powernv_cpufreq_get(), cpu_sibling_mask() suffices
since we are using this mask as a parameter to
smp_call_function_any(), and hence are concerned only about the online
siblings. 

I shall fix the changelog to reflect Srivatsa's reason for having a
mask that does not vary with cpu-hotplug.

> 
> Regards,
> Srivatsa S. Bhat
> 
> 
> > Also, this should have been CCed to the cpufreq mailing list and maybe
> > the relevant maintainer too.

Ok. Will do it in the subsequent version. 

Thanks for the review!

> > 
> > Cheers,
> > Ben.
> > 
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> >> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> >> ---
> >>  drivers/cpufreq/powernv-cpufreq.c | 24 ++++++++++++++++++------
> >>  1 file changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> >> index ab1551f..4cad727 100644
> >> --- a/drivers/cpufreq/powernv-cpufreq.c
> >> +++ b/drivers/cpufreq/powernv-cpufreq.c
> >> @@ -115,6 +115,23 @@ static struct freq_attr *powernv_cpu_freq_attr[] = {
> >>  
> >>  /* Helper routines */
> >>  
> >> +/**
> >> + * Sets the bits corresponding to the thread-siblings of cpu in its core
> >> + * in 'cpus'.
> >> + */
> >> +static void powernv_cpu_to_core_mask(unsigned int cpu, cpumask_var_t cpus)
> >> +{
> >> +	int base, i;
> >> +
> >> +	base = cpu_first_thread_sibling(cpu);
> >> +
> >> +	for (i = 0; i < threads_per_core; i++) {
> >> +		cpumask_set_cpu(base + i, cpus);
> >> +	}
> >> +
> >> +	return;
> >> +}
> >> +
> >>  /* Access helpers to power mgt SPR */
> >>  
> >>  static inline unsigned long get_pmspr(unsigned long sprn)
> >> @@ -180,13 +197,8 @@ static int powernv_set_freq(cpumask_var_t cpus, unsigned int new_index)
> >>  
> >>  static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >>  {
> >> -	int base, i;
> >> -
> >>  #ifdef CONFIG_SMP
> >> -	base = cpu_first_thread_sibling(policy->cpu);
> >> -
> >> -	for (i = 0; i < threads_per_core; i++)
> >> -		cpumask_set_cpu(base + i, policy->cpus);
> >> +	powernv_cpu_to_core_mask(policy->cpu, policy->cpus);
> >>  #endif
> >>  	policy->cpuinfo.transition_latency = 25000;
> >>  
> > 
> > 
>

Patch

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index ab1551f..4cad727 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -115,6 +115,23 @@  static struct freq_attr *powernv_cpu_freq_attr[] = {
 
 /* Helper routines */
 
+/**
+ * Sets the bits corresponding to the thread-siblings of cpu in its core
+ * in 'cpus'.
+ */
+static void powernv_cpu_to_core_mask(unsigned int cpu, cpumask_var_t cpus)
+{
+	int base, i;
+
+	base = cpu_first_thread_sibling(cpu);
+
+	for (i = 0; i < threads_per_core; i++) {
+		cpumask_set_cpu(base + i, cpus);
+	}
+
+	return;
+}
+
 /* Access helpers to power mgt SPR */
 
 static inline unsigned long get_pmspr(unsigned long sprn)
@@ -180,13 +197,8 @@  static int powernv_set_freq(cpumask_var_t cpus, unsigned int new_index)
 
 static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-	int base, i;
-
 #ifdef CONFIG_SMP
-	base = cpu_first_thread_sibling(policy->cpu);
-
-	for (i = 0; i < threads_per_core; i++)
-		cpumask_set_cpu(base + i, policy->cpus);
+	powernv_cpu_to_core_mask(policy->cpu, policy->cpus);
 #endif
 	policy->cpuinfo.transition_latency = 25000;