| Message ID | 1394449861-8688-3-git-send-email-ego@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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; >
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; >> > >
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; > >> > > > > >
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;