| Message ID | b8dc587ec7767b89b1d24f1348acd1a341e54a4a.1444583718.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 10/11/2015 10:21 AM, Viresh Kumar wrote: > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> The commit text should explain the why you are doing this. > --- > drivers/cpufreq/cpufreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 25c4c15103a0..b32521432db4 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1221,7 +1221,7 @@ static int cpufreq_online(unsigned int cpu) > > if (new_policy) { > /* related_cpus should at least include policy->cpus. */ > - cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); > + cpumask_copy(policy->related_cpus, policy->cpus); Again, why? It actually seems wrong. A 4 core cluster could come up with just 2 cores when the policy is added. But the related CPUs would be 4 CPUs. > /* Remember CPUs present at the policy creation time. */ > cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask); > } > -Saravana
On 12-10-15, 12:12, Saravana Kannan wrote: > > if (new_policy) { > > /* related_cpus should at least include policy->cpus. */ > >- cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); > >+ cpumask_copy(policy->related_cpus, policy->cpus); > > Again, why? It actually seems wrong. A 4 core cluster could come up > with just 2 cores when the policy is added. But the related CPUs > would be 4 CPUs. Firstly, the patch hasn't changed anything at all. related_cpus was empty until this point, and orring or setting it with ->cpus will result in the same output. Secondly, this is what we always wanted. related_cpus should contain the mask of all possible CPUs for that cluster.
On 10/12/2015 08:23 PM, Viresh Kumar wrote: > On 12-10-15, 12:12, Saravana Kannan wrote: >>> if (new_policy) { >>> /* related_cpus should at least include policy->cpus. */ >>> - cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); >>> + cpumask_copy(policy->related_cpus, policy->cpus); >> >> Again, why? It actually seems wrong. A 4 core cluster could come up >> with just 2 cores when the policy is added. But the related CPUs >> would be 4 CPUs. > > Firstly, the patch hasn't changed anything at all. related_cpus was > empty until this point, and orring or setting it with ->cpus will > result in the same output. I was under the impression that the CPUfreq drivers were expected to fill in related_cpus and the or-ing was a safety net. If that's not the case, then this change is fine. > Secondly, this is what we always wanted. related_cpus should contain > the mask of all possible CPUs for that cluster. I think the confusion was that I thought the drivers are supposed to do this. If this doesn't mess up other CPUfreq drivers that I'm not familiar with, then I don't have concerns. Can you still explain the why in the commit text though? If it's just that related_cpus is always empty and copying is more efficient than or-ing, then mention that? Thanks, Saravana
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 25c4c15103a0..b32521432db4 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1221,7 +1221,7 @@ static int cpufreq_online(unsigned int cpu) if (new_policy) { /* related_cpus should at least include policy->cpus. */ - cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); + cpumask_copy(policy->related_cpus, policy->cpus); /* Remember CPUs present at the policy creation time. */ cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask); }
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)