[git-pull -tip] x86:perf_counter cleanup

[git-pull -tip] x86:perf_counter cleanup

[Jaswinder Singh Rajput]

The following changes since commit 880860e392d92c457e8116cdee39ec4d109174ee:
  Paul Mackerras (1):
        perfcounters/powerpc: add support for POWER4 processors

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tip.git perfcounters/core

Jaswinder Singh Rajput (2):
      x86: perf_counter.c remove unused variables
      x86: perf_counter.c fix style problems

 arch/x86/kernel/cpu/perf_counter.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

Complete diff:
diff --git a/arch/x86/kernel/cpu/perf_counter.c b/arch/x86/kernel/cpu/perf_counter.c
index f585371..7460895 100644
--- a/arch/x86/kernel/cpu/perf_counter.c
+++ b/arch/x86/kernel/cpu/perf_counter.c
@@ -17,7 +17,6 @@
 #include <linux/kdebug.h>
 #include <linux/sched.h>
 
-#include <asm/perf_counter.h>
 #include <asm/apic.h>
 
 static bool perf_counters_initialized __read_mostly;
@@ -263,6 +262,9 @@ static u64 pmc_amd_save_disable_all(void)
 	return enabled;
 }
 
+/*
+ * Exported because of ACPI idle
+ */
 u64 hw_perf_save_disable(void)
 {
 	if (unlikely(!perf_counters_initialized))
@@ -270,9 +272,6 @@ u64 hw_perf_save_disable(void)
 
 	return pmc_ops->save_disable_all();
 }
-/*
- * Exported because of ACPI idle
- */
 EXPORT_SYMBOL_GPL(hw_perf_save_disable);
 
 static void pmc_intel_restore_all(u64 ctrl)
@@ -301,6 +300,9 @@ static void pmc_amd_restore_all(u64 ctrl)
 	}
 }
 
+/*
+ * Exported because of ACPI idle
+ */
 void hw_perf_restore(u64 ctrl)
 {
 	if (unlikely(!perf_counters_initialized))
@@ -308,9 +310,6 @@ void hw_perf_restore(u64 ctrl)
 
 	pmc_ops->restore_all(ctrl);
 }
-/*
- * Exported because of ACPI idle
- */
 EXPORT_SYMBOL_GPL(hw_perf_restore);
 
 static u64 pmc_intel_get_status(u64 mask)
@@ -954,9 +953,6 @@ static struct pmc_x86_ops *pmc_intel_init(void)
 
 static struct pmc_x86_ops *pmc_amd_init(void)
 {
-	u64 old;
-	int bits;
-
 	nr_counters_generic = 4;
 	nr_counters_fixed = 0;
 	counter_value_mask = 0x0000FFFFFFFFFFFFULL;

Re: [git-pull -tip] x86:perf_counter cleanup

[Peter Zijlstra]

On Sun, 2009-03-08 at 15:43 +0530, Jaswinder Singh Rajput wrote:

> +/*
> + * Exported because of ACPI idle
> + */
>  u64 hw_perf_save_disable(void)
>  {
>  	if (unlikely(!perf_counters_initialized))
> @@ -270,9 +272,6 @@ u64 hw_perf_save_disable(void)
>  
>  	return pmc_ops->save_disable_all();
>  }
> -/*
> - * Exported because of ACPI idle
> - */
>  EXPORT_SYMBOL_GPL(hw_perf_save_disable);
>  
>  static void pmc_intel_restore_all(u64 ctrl)
> @@ -301,6 +300,9 @@ static void pmc_amd_restore_all(u64 ctrl)
>  	}
>  }
>  
> +/*
> + * Exported because of ACPI idle
> + */
>  void hw_perf_restore(u64 ctrl)
>  {
>  	if (unlikely(!perf_counters_initialized))
> @@ -308,9 +310,6 @@ void hw_perf_restore(u64 ctrl)
>  
>  	pmc_ops->restore_all(ctrl);
>  }
> -/*
> - * Exported because of ACPI idle
> - */
>  EXPORT_SYMBOL_GPL(hw_perf_restore);

Please leave those comments where they are, they do not describe the
function, but comment on the export.

Re: [git-pull -tip] x86:perf_counter cleanup

[Jaswinder Singh Rajput]

Hello Peter,

On Sun, 2009-03-08 at 11:20 +0100, Peter Zijlstra wrote:
> On Sun, 2009-03-08 at 15:43 +0530, Jaswinder Singh Rajput wrote:
> 
> > +/*
> > + * Exported because of ACPI idle
> > + */
> >  u64 hw_perf_save_disable(void)
> >  {
> >  	if (unlikely(!perf_counters_initialized))
> > @@ -270,9 +272,6 @@ u64 hw_perf_save_disable(void)
> >  
> >  	return pmc_ops->save_disable_all();
> >  }
> > -/*
> > - * Exported because of ACPI idle
> > - */
> >  EXPORT_SYMBOL_GPL(hw_perf_save_disable);
> >  
> >  static void pmc_intel_restore_all(u64 ctrl)
> > @@ -301,6 +300,9 @@ static void pmc_amd_restore_all(u64 ctrl)
> >  	}
> >  }
> >  
> > +/*
> > + * Exported because of ACPI idle
> > + */
> >  void hw_perf_restore(u64 ctrl)
> >  {
> >  	if (unlikely(!perf_counters_initialized))
> > @@ -308,9 +310,6 @@ void hw_perf_restore(u64 ctrl)
> >  
> >  	pmc_ops->restore_all(ctrl);
> >  }
> > -/*
> > - * Exported because of ACPI idle
> > - */
> >  EXPORT_SYMBOL_GPL(hw_perf_restore);
> 
> Please leave those comments where they are, they do not describe the
> function, but comment on the export.

Have you checked this:
http://git.kernel.org/?p=linux/kernel/git/jaswinder/linux-2.6-tip.git;a=commitdiff;h=1ae843834668af3a30484e256fb27dc1a49f59cb

These leads to warnings:
 WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
 WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable

So we have two options:
1. which I already shown above
2. remove these comments

Please let me know, which option you will prefer.

Thanks,
--
JSR

Re: [git-pull -tip] x86:perf_counter cleanup

[Ingo Molnar]

* Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:

> Hello Peter,
> 
> On Sun, 2009-03-08 at 11:20 +0100, Peter Zijlstra wrote:
> > On Sun, 2009-03-08 at 15:43 +0530, Jaswinder Singh Rajput wrote:
> > 
> > > +/*
> > > + * Exported because of ACPI idle
> > > + */
> > >  u64 hw_perf_save_disable(void)
> > >  {
> > >  	if (unlikely(!perf_counters_initialized))
> > > @@ -270,9 +272,6 @@ u64 hw_perf_save_disable(void)
> > >  
> > >  	return pmc_ops->save_disable_all();
> > >  }
> > > -/*
> > > - * Exported because of ACPI idle
> > > - */
> > >  EXPORT_SYMBOL_GPL(hw_perf_save_disable);
> > >  
> > >  static void pmc_intel_restore_all(u64 ctrl)
> > > @@ -301,6 +300,9 @@ static void pmc_amd_restore_all(u64 ctrl)
> > >  	}
> > >  }
> > >  
> > > +/*
> > > + * Exported because of ACPI idle
> > > + */
> > >  void hw_perf_restore(u64 ctrl)
> > >  {
> > >  	if (unlikely(!perf_counters_initialized))
> > > @@ -308,9 +310,6 @@ void hw_perf_restore(u64 ctrl)
> > >  
> > >  	pmc_ops->restore_all(ctrl);
> > >  }
> > > -/*
> > > - * Exported because of ACPI idle
> > > - */
> > >  EXPORT_SYMBOL_GPL(hw_perf_restore);
> > 
> > Please leave those comments where they are, they do not describe the
> > function, but comment on the export.
> 
> Have you checked this:
> http://git.kernel.org/?p=linux/kernel/git/jaswinder/linux-2.6-tip.git;a=commitdiff;h=1ae843834668af3a30484e256fb27dc1a49f59cb
> 
> These leads to warnings:
>  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable

ignore those checkpatch warnings, they are wrong in this case.

> So we have two options:
> 1. which I already shown above
> 2. remove these comments

  3. ignore incorrect warnings

	Ingo

Re: [git-pull -tip] x86:perf_counter cleanup

[Jaswinder Singh Rajput]

On Sun, 2009-03-08 at 11:41 +0100, Ingo Molnar wrote:
> * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:
> > These leads to warnings:
> >  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> >  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> 
> ignore those checkpatch warnings, they are wrong in this case.

OK, ignored for time being.

Log messages available at:
http://git.kernel.org/?p=linux/kernel/git/jaswinder/linux-2.6-tip.git;a=shortlog;h=perfcounters/core

So here is new pull request:

The following changes since commit 880860e392d92c457e8116cdee39ec4d109174ee:
  Paul Mackerras (1):
        perfcounters/powerpc: add support for POWER4 processors

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tip.git perfcounters/core

Jaswinder Singh Rajput (2):
      x86: perf_counter.c remove unused variables
      x86: perf_counter.c remove duplicate header file

 arch/x86/kernel/cpu/perf_counter.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

Complete diff:
diff --git a/arch/x86/kernel/cpu/perf_counter.c b/arch/x86/kernel/cpu/perf_counter.c
index f585371..79d88bf 100644
--- a/arch/x86/kernel/cpu/perf_counter.c
+++ b/arch/x86/kernel/cpu/perf_counter.c
@@ -17,7 +17,6 @@
 #include <linux/kdebug.h>
 #include <linux/sched.h>
 
-#include <asm/perf_counter.h>
 #include <asm/apic.h>
 
 static bool perf_counters_initialized __read_mostly;
@@ -954,9 +953,6 @@ static struct pmc_x86_ops *pmc_intel_init(void)
 
 static struct pmc_x86_ops *pmc_amd_init(void)
 {
-	u64 old;
-	int bits;
-
 	nr_counters_generic = 4;
 	nr_counters_fixed = 0;
 	counter_value_mask = 0x0000FFFFFFFFFFFFULL;

Re: [git-pull -tip] x86:perf_counter cleanup

[Peter Zijlstra]

On Sun, 2009-03-08 at 16:09 +0530, Jaswinder Singh Rajput wrote:
> These leads to warnings:
>  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable

Its the exception that makes the rule ;-)

Re: [git-pull -tip] x86:perf_counter cleanup

[Peter Zijlstra]

On Sun, 2009-03-08 at 15:43 +0530, Jaswinder Singh Rajput wrote:
>  
>  static struct pmc_x86_ops *pmc_amd_init(void)
>  {
> -       u64 old;
> -       int bits;
> -
>         nr_counters_generic = 4;
>         nr_counters_fixed = 0;
>         counter_value_mask = 0x0000FFFFFFFFFFFFULL;



I would rather we try something like the below:

diff --git a/arch/x86/kernel/cpu/perf_counter.c b/arch/x86/kernel/cpu/perf_counter.c
index f585371..ccb4f79 100644
--- a/arch/x86/kernel/cpu/perf_counter.c
+++ b/arch/x86/kernel/cpu/perf_counter.c
@@ -957,12 +957,39 @@ static struct pmc_x86_ops *pmc_amd_init(void)
 	u64 old;
 	int bits;
 
+	pr_info("AMD Performance Monitoring support detected.\n");
+
 	nr_counters_generic = 4;
 	nr_counters_fixed = 0;
-	counter_value_mask = 0x0000FFFFFFFFFFFFULL;
-	counter_value_bits = 48;
+	counter_value_mask = ~0ULL;
 
-	pr_info("AMD Performance Monitoring support detected.\n");
+	rdmsrl(MSR_K7_PERFCTR0, old);
+	wrmsrl(MSR_K7_PERFCTR0, counter_value_mask);
+
+	/*
+	 * Ensure the counter_value_mask write, is completed before
+	 * we try to read it back.
+	 */ 
+	mb();
+
+	/* read the truncated mask */
+	rdmsrl(MSR_K7_PERFCTR0, counter_value_mask);
+	wrmsrl(MSR_K7_PERFCTR0, old);
+
+	bits = 32 + fls(counter_value_mask >> 32);
+	if (bits == 32)
+		bits = fls((u32)counter_value_mask);
+	counter_value_bits = bits;
+
+	if (counter_value_bits != 48 ||
+	    counter_value_mask != ((1ULL << counter_value_bits) - 1)) {
+		printk(KERN_ERR "Bad cpu! speculated through full memory barrier!\n");
+		printk(KERN_ERR " ... old value: %llx\n", old);
+		printk(KERN_ERR " ... new value: %llx\n", counter_value_mask);
+
+		counter_value_bits = 48;
+		counter_value_mask = 0x0000FFFFFFFFFFFFULL;
+	}
 
 	return &pmc_amd_ops;
 }

Re: [git-pull -tip] x86:perf_counter cleanup

[Peter Zijlstra]

An actual cleanup would be:

---
Subject: perf_counters: cleanup amd64 code a little
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Thu Mar 05 21:18:49 CET 2009

Use and actual unsigned long bitmap instead of casting our way around.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 arch/x86/kernel/cpu/perf_counter.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6/arch/x86/kernel/cpu/perf_counter.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/perf_counter.c
+++ linux-2.6/arch/x86/kernel/cpu/perf_counter.c
@@ -37,7 +37,7 @@ struct cpu_hw_counters {
 	unsigned long		used[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
 	unsigned long		interrupts;
 	u64			throttle_ctrl;
-	u64			active_mask;
+	unsigned long		active_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
 	int			enabled;
 };
 
@@ -291,7 +291,7 @@ static void pmc_amd_restore_all(u64 ctrl
 		return;
 
 	for (idx = 0; idx < nr_counters_generic; idx++) {
-		if (test_bit(idx, (unsigned long *)&cpuc->active_mask)) {
+		if (test_bit(idx, cpuc->active_mask)) {
 			u64 val;
 
 			rdmsrl(MSR_K7_EVNTSEL0 + idx, val);
@@ -377,7 +377,7 @@ static void pmc_amd_enable(int idx, u64 
 {
 	struct cpu_hw_counters *cpuc = &__get_cpu_var(cpu_hw_counters);
 
-	set_bit(idx, (unsigned long *)&cpuc->active_mask);
+	set_bit(idx, cpuc->active_mask);
 	if (cpuc->enabled)
 		config |= ARCH_PERFMON_EVENTSEL0_ENABLE;
 
@@ -401,7 +401,7 @@ static void pmc_amd_disable(int idx, u64
 {
 	struct cpu_hw_counters *cpuc = &__get_cpu_var(cpu_hw_counters);
 
-	clear_bit(idx, (unsigned long *)&cpuc->active_mask);
+	clear_bit(idx, cpuc->active_mask);
 	wrmsrl(MSR_K7_EVNTSEL0 + idx, config);
 
 }

[tip:perfcounters/core] x86: perf_counter cleanup

[Peter Zijlstra]

Commit-ID:  184fe4ab1f2e4dfa45584889bb3820031648386b
Gitweb:     http://git.kernel.org/tip/184fe4ab1f2e4dfa45584889bb3820031648386b
Author:     "Peter Zijlstra" <peterz@infradead.org>
AuthorDate: Sun, 8 Mar 2009 11:34:19 +0100
Commit:     Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 8 Mar 2009 16:24:49 +0100

x86: perf_counter cleanup

Use and actual unsigned long bitmap instead of casting our way around.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jaswinder Singh Rajput <jaswinder@kernel.org>
LKML-Reference: <1236508459.22914.3645.camel@twins>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/cpu/perf_counter.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_counter.c b/arch/x86/kernel/cpu/perf_counter.c
index f585371..1df4210 100644
--- a/arch/x86/kernel/cpu/perf_counter.c
+++ b/arch/x86/kernel/cpu/perf_counter.c
@@ -37,7 +37,7 @@ struct cpu_hw_counters {
 	unsigned long		used[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
 	unsigned long		interrupts;
 	u64			throttle_ctrl;
-	u64			active_mask;
+	unsigned long		active_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
 	int			enabled;
 };
 
@@ -291,7 +291,7 @@ static void pmc_amd_restore_all(u64 ctrl)
 		return;
 
 	for (idx = 0; idx < nr_counters_generic; idx++) {
-		if (test_bit(idx, (unsigned long *)&cpuc->active_mask)) {
+		if (test_bit(idx, cpuc->active_mask)) {
 			u64 val;
 
 			rdmsrl(MSR_K7_EVNTSEL0 + idx, val);
@@ -377,7 +377,7 @@ static void pmc_amd_enable(int idx, u64 config)
 {
 	struct cpu_hw_counters *cpuc = &__get_cpu_var(cpu_hw_counters);
 
-	set_bit(idx, (unsigned long *)&cpuc->active_mask);
+	set_bit(idx, cpuc->active_mask);
 	if (cpuc->enabled)
 		config |= ARCH_PERFMON_EVENTSEL0_ENABLE;
 
@@ -401,7 +401,7 @@ static void pmc_amd_disable(int idx, u64 config)
 {
 	struct cpu_hw_counters *cpuc = &__get_cpu_var(cpu_hw_counters);
 
-	clear_bit(idx, (unsigned long *)&cpuc->active_mask);
+	clear_bit(idx, cpuc->active_mask);
 	wrmsrl(MSR_K7_EVNTSEL0 + idx, config);
 
 }

Re: [tip:perfcounters/core] x86: perf_counter cleanup

[Peter Zijlstra]

On Sun, 2009-03-08 at 15:52 +0000, Jaswinder Singh Rajput wrote:
> Commit-ID:  e255357764f92afcafafbd4879b222b8c752065a
> Gitweb:     http://git.kernel.org/tip/e255357764f92afcafafbd4879b222b8c752065a
> Author:     "Jaswinder Singh Rajput" <jaswinder@kernel.org>
> AuthorDate: Sun, 8 Mar 2009 17:09:49 +0530
> Commit:     Ingo Molnar <mingo@elte.hu>
> CommitDate: Sun, 8 Mar 2009 16:26:50 +0100
> 
> x86: perf_counter cleanup
> 
> Remove unused variables and duplicate header file.

Why did you commit this?

Until someone from AMD steps up and either tells us they're going to be
48bits _ALWAYS_ or that it's impossible to detect dynamically, I'd
rather try and get the dynamic thing working.

> Signed-off-by: Jaswinder Singh Rajput <jaswinder@kernel.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> ---
>  arch/x86/kernel/cpu/perf_counter.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_counter.c b/arch/x86/kernel/cpu/perf_counter.c
> index 1df4210..155bc3c 100644
> --- a/arch/x86/kernel/cpu/perf_counter.c
> +++ b/arch/x86/kernel/cpu/perf_counter.c
> @@ -17,7 +17,6 @@
>  #include <linux/kdebug.h>
>  #include <linux/sched.h>
>  
> -#include <asm/perf_counter.h>
>  #include <asm/apic.h>
>  
>  static bool perf_counters_initialized __read_mostly;
> @@ -954,9 +953,6 @@ static struct pmc_x86_ops *pmc_intel_init(void)
>  
>  static struct pmc_x86_ops *pmc_amd_init(void)
>  {
> -	u64 old;
> -	int bits;
> -
>  	nr_counters_generic = 4;
>  	nr_counters_fixed = 0;
>  	counter_value_mask = 0x0000FFFFFFFFFFFFULL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tip-commits" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [tip:perfcounters/core] x86: perf_counter cleanup

[H. Peter Anvin]

Peter Zijlstra wrote:
> On Sun, 2009-03-08 at 15:52 +0000, Jaswinder Singh Rajput wrote:
>> Commit-ID:  e255357764f92afcafafbd4879b222b8c752065a
>> Gitweb:     http://git.kernel.org/tip/e255357764f92afcafafbd4879b222b8c752065a
>> Author:     "Jaswinder Singh Rajput" <jaswinder@kernel.org>
>> AuthorDate: Sun, 8 Mar 2009 17:09:49 +0530
>> Commit:     Ingo Molnar <mingo@elte.hu>
>> CommitDate: Sun, 8 Mar 2009 16:26:50 +0100
>>
>> x86: perf_counter cleanup
>>
>> Remove unused variables and duplicate header file.
> 
> Why did you commit this?
> 
> Until someone from AMD steps up and either tells us they're going to be
> 48bits _ALWAYS_ or that it's impossible to detect dynamically, I'd
> rather try and get the dynamic thing working.
> 

That would be good, but there really is absolutely no reason to leave in
variables which are unused in the current code and therefore give a gcc
warning.  It's not like adding new declaration is hard.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [tip:perfcounters/core] x86: perf_counter cleanup

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Peter Zijlstra wrote:
> > On Sun, 2009-03-08 at 15:52 +0000, Jaswinder Singh Rajput wrote:
> >> Commit-ID:  e255357764f92afcafafbd4879b222b8c752065a
> >> Gitweb:     http://git.kernel.org/tip/e255357764f92afcafafbd4879b222b8c752065a
> >> Author:     "Jaswinder Singh Rajput" <jaswinder@kernel.org>
> >> AuthorDate: Sun, 8 Mar 2009 17:09:49 +0530
> >> Commit:     Ingo Molnar <mingo@elte.hu>
> >> CommitDate: Sun, 8 Mar 2009 16:26:50 +0100
> >>
> >> x86: perf_counter cleanup
> >>
> >> Remove unused variables and duplicate header file.
> > 
> > Why did you commit this?
> > 
> > Until someone from AMD steps up and either tells us they're 
> > going to be 48bits _ALWAYS_ or that it's impossible to 
> > detect dynamically, I'd rather try and get the dynamic thing 
> > working.
> 
> That would be good, but there really is absolutely no reason 
> to leave in variables which are unused in the current code and 
> therefore give a gcc warning.  It's not like adding new 
> declaration is hard.

yeah. Adding a TODO there might be worth doing though.

	Ingo