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

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

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);
 
 }