| Message ID | 564c8b1f073dc396b54866a2220b09b7869289f4.1379779777.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/22/2013 03:21 AM, Viresh Kumar wrote: > In cpuidle_coupled_register_device() we do following: > if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) > coupled->prevent++; > > This is only required to be done when we are using 'coupled' from an existing > cpuidle_device and not when we have just done this: > > coupled->coupled_cpus = dev->coupled_cpus > > So, move this compare statement to the right place. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Cc'ed Colin Cross. > --- > drivers/cpuidle/coupled.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c > index e952936..19a89eb 100644 > --- a/drivers/cpuidle/coupled.c > +++ b/drivers/cpuidle/coupled.c > @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) > other_dev = per_cpu(cpuidle_devices, cpu); > if (other_dev && other_dev->coupled) { > coupled = other_dev->coupled; > + > + if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, > + &coupled->coupled_cpus))) > + coupled->prevent++; > goto have_coupled; > } > } > @@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) > > have_coupled: > dev->coupled = coupled; > - if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) > - coupled->prevent++; > - > cpuidle_coupled_update_online_cpus(coupled); > > coupled->refcnt++; >
On Sat, Sep 21, 2013 at 6:21 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > In cpuidle_coupled_register_device() we do following: > if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) > coupled->prevent++; > > This is only required to be done when we are using 'coupled' from an existing > cpuidle_device and not when we have just done this: > > coupled->coupled_cpus = dev->coupled_cpus > > So, move this compare statement to the right place. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> I don't agree with this. This patch is a tiny optimization in code that is rarely called, and it moves a final sanity check somewhere that it might get missed if the code were later refactored. > --- > drivers/cpuidle/coupled.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c > index e952936..19a89eb 100644 > --- a/drivers/cpuidle/coupled.c > +++ b/drivers/cpuidle/coupled.c > @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) > other_dev = per_cpu(cpuidle_devices, cpu); > if (other_dev && other_dev->coupled) { > coupled = other_dev->coupled; > + > + if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, > + &coupled->coupled_cpus))) > + coupled->prevent++; > goto have_coupled; > } > } > @@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) > > have_coupled: > dev->coupled = coupled; > - if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) > - coupled->prevent++; > - > cpuidle_coupled_update_online_cpus(coupled); > > coupled->refcnt++; > -- > 1.7.12.rc2.18.g61b472e > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 26 September 2013 05:55, Colin Cross <ccross@google.com> wrote: > I don't agree with this. This patch is a tiny optimization in code > that is rarely called, and it moves a final sanity check somewhere > that it might get missed if the code were later refactored. This is what we are doing for the first cpu of coupled-cpus: if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &dev->coupled_cpus))) coupled->prevent++; i.e. comparing a variable to itself :) And I believe my patch puts the sanity check at the right place (where we are using coupled from existing CPUs.. And that is where it should have been since the beginning).. If people miss this during code re-factoring, then it would be even more stupid on the part of Author and Reviewer.. And if it still gets missed then this is not the only place where we need to worry about such stuff.. This is present everywhere in our code.. You can't really some part of code to some place and leave the other as-is.. The change is supposed to be more logical and so funny mistakes must be caught during reviews.
On Thu, Sep 26, 2013 at 1:36 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 26 September 2013 05:55, Colin Cross <ccross@google.com> wrote: >> I don't agree with this. This patch is a tiny optimization in code >> that is rarely called, and it moves a final sanity check somewhere >> that it might get missed if the code were later refactored. > > This is what we are doing for the first cpu of coupled-cpus: > if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &dev->coupled_cpus))) > coupled->prevent++; > > i.e. comparing a variable to itself :) > > And I believe my patch puts the sanity check at the right place (where > we are using coupled from existing CPUs.. And that is where it should > have been since the beginning).. > > If people miss this during code re-factoring, then it would be even more > stupid on the part of Author and Reviewer.. And if it still gets missed > then this is not the only place where we need to worry about such stuff.. > > This is present everywhere in our code.. You can't really some part of > code to some place and leave the other as-is.. The change is supposed > to be more logical and so funny mistakes must be caught during reviews. It's fine where it is. Once dev and coupled are both known, regardless of how they were found, it performs a final sanity check that nothing went wrong. Moving into a specific branch of the finding code defeats the purpose. Yes, it performs a useless check on the first cpu, but it keeps the code readable and maintainable, so it stays where it is. NAK.
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c index e952936..19a89eb 100644 --- a/drivers/cpuidle/coupled.c +++ b/drivers/cpuidle/coupled.c @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) other_dev = per_cpu(cpuidle_devices, cpu); if (other_dev && other_dev->coupled) { coupled = other_dev->coupled; + + if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, + &coupled->coupled_cpus))) + coupled->prevent++; goto have_coupled; } } @@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) have_coupled: dev->coupled = coupled; - if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) - coupled->prevent++; - cpuidle_coupled_update_online_cpus(coupled); coupled->refcnt++;
In cpuidle_coupled_register_device() we do following: if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) coupled->prevent++; This is only required to be done when we are using 'coupled' from an existing cpuidle_device and not when we have just done this: coupled->coupled_cpus = dev->coupled_cpus So, move this compare statement to the right place. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpuidle/coupled.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)