Skip to contentSkip to conversation

OProfile fixes for v2.6.28

GIT PULL In review Robert Richter 5 participants 17 messages Nov 7, 2008 – Nov 18, 2008 x86

lore
Share & tools
lore

Robert Richter

author
#0 lore raw

Hi Ingo,

please pull oprofile fixes for 2.6.28 for tip regression:

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git oprofile-for-tip

Thanks,

-Robert

Andi Kleen (1):
oprofile: Fix p6 counter overflow check

Carl Love (2):
Cell OProfile: Incorrect local array size in activate spu profiling function Cell OProfile: Incorrect local array size in activate spu profiling function

Jesper Dangaard Brouer (1):
Change UTF8 chars in Kconfig help text about Oprofile AMD barcelona

Nick Piggin (1):
oprofile: fix memory ordering

Robert Richter (1):
Revert "Cell OProfile: Incorrect local array size in activate spu profiling function"

Signature · 3 lines
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]
#1 lore raw
Quoted 7 lines · Robert Richter

Hi Ingo,

please pull oprofile fixes for 2.6.28 for tip regression:

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git oprofile-for-tip

Thanks,

pulled, thanks Robert!

Ingo

#2 lore raw
In reply to Robert Richter
Quoted 17 lines · Robert Richter

Hi Ingo,

please pull oprofile fixes for 2.6.28 for tip regression:

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git oprofile-for-tip

Thanks,

-Robert

Andi Kleen (1):
oprofile: Fix p6 counter overflow check

Carl Love (2):
Cell OProfile: Incorrect local array size in activate spu profiling function Cell OProfile: Incorrect local array size in activate spu profiling function

Quoted 9 lines

Jesper Dangaard Brouer (1):
Change UTF8 chars in Kconfig help text about Oprofile AMD barcelona

Nick Piggin (1):
oprofile: fix memory ordering

Robert Richter (1):
Revert "Cell OProfile: Incorrect local array size in activate spu profiling function"

Hi Robert

I am still trying to find why oprofile stops after some millions of samples on my machine.

(All versions of linux supporting oprofile/NMI on this machine have this problem) Note : I am using a 32bit kernel.

While doing code review I found this bug.

Impact: 32bit kernels, memory corruption

[PATCH] oprofile: fix an overflow in ppro code

reset_value was changed from long to u64 in commit b99170288421c79f0c2efa8b33e26e65f4bb7fb8 (oprofile: Implement Intel architectural perfmon support)

But dynamic allocation of this array use a wrong type (long instead of u64)

Signed-off-by Eric Dumazet <[email protected]>

---
arch/x86/oprofile/op_model_ppro.c | 2 +-

1 files changed, 1 insertion(+), 1 deletion(-)
arch/x86/oprofile/op_model_ppro.c +1 -1
index 3f1b81a..716d26f 100644
@@ -69,7 +69,7 @@ static void ppro_setup_ctrs(struct op_msrs const * const msrs)
 	int i;
 
 	if (!reset_value) {
-		reset_value = kmalloc(sizeof(unsigned) * num_counters,
+		reset_value = kmalloc(sizeof(reset_value[0]) * num_counters,
 					GFP_ATOMIC);
 		if (!reset_value)
 			return;
#3 lore raw

arch/x86/oprofile/op_model_ppro.c static void ppro_setup_ctrs(struct op_msrs const * const msrs) · line 72

Quoted 10 lines · Eric Dumazet
arch/x86/oprofile/op_model_ppro.c +1 -1
index 3f1b81a..716d26f 100644
@@ -69,7 +69,7 @@ static void ppro_setup_ctrs(struct op_msrs const * const msrs)
 	int i;
 
 	if (!reset_value) {
-		reset_value = kmalloc(sizeof(unsigned) * num_counters,
+		reset_value = kmalloc(sizeof(reset_value[0]) * num_counters,

Thanks for tracking this down.

But that still doesn't explain why 2.6.27 fails too?

-Andi

Signature · 1 line
[email protected]
#4 lore raw
Quoted 9 lines · Andi Kleen
Quoted 7 lines · Eric Dumazet
arch/x86/oprofile/op_model_ppro.c +0 -0
index 3f1b81a..716d26f 100644
@@ -69,7 +69,7 @@ static void ppro_setup_ctrs(struct op_msrs const * const msrs)
 	int i;
Quoted 7 lines

arch/x86/oprofile/op_model_ppro.c static void ppro_setup_ctrs(struct op_msrs const * const msrs) · line 72

Quoted 3 lines
arch/x86/oprofile/op_model_ppro.c +1 -1
 	if (!reset_value) {
-		reset_value = kmalloc(sizeof(unsigned) * num_counters,
+		reset_value = kmalloc(sizeof(reset_value[0]) * num_counters,

Thanks for tracking this down.

But that still doesn't explain why 2.6.27 fails too?

No :(

In fact 2.6.24 fails too. Oprofile never worked correctly.

Still investigating...

#5 lore raw

PATCH 1/1 oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

In reply to Andi Kleen
Quoted 9 lines · Andi Kleen
Quoted 7 lines · Eric Dumazet
arch/x86/oprofile/op_model_ppro.c +0 -0
index 3f1b81a..716d26f 100644
@@ -69,7 +69,7 @@ static void ppro_setup_ctrs(struct op_msrs const * const msrs)
 	int i;
Quoted 7 lines

arch/x86/oprofile/op_model_ppro.c static void ppro_setup_ctrs(struct op_msrs const * const msrs) · line 72

Quoted 3 lines
arch/x86/oprofile/op_model_ppro.c +1 -1
 	if (!reset_value) {
-		reset_value = kmalloc(sizeof(unsigned) * num_counters,
+		reset_value = kmalloc(sizeof(reset_value[0]) * num_counters,

Thanks for tracking this down.

But that still doesn't explain why 2.6.27 fails too?

Desesperatly Seeking Oprofile, next round.

I know *nothing* about APIC but spent few hours to try several tricks and finally found something.

It solved my problem : oprofile can run several hours without any freeze of NMI on any core.

# grep NMI /proc/interrupts
NMI:   10902884    9635871   10333815    8372989    7971483    8298373    8877495   10206963   Non-maskable interrupts
...
# grep NMI /proc/interrupts
NMI:   15518834   14340713   15038694   13078235   12676585   13003394   13582115   14912146   Non-maskable interrupts

Can anybody understand and explain what is happening ?

Is it a software or hardware problem ?

[PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

While using oprofile on my HP BL460c G1, (two quad core intel E5450 CPU), I noticed that one CPU after the other could not get anymore NMI.

After a while, all cores where blocked (ie not generating events for oprofile) I tried all major linux versions and all where affected by this freeze.

I found that we have to re-arm APIC_DM_NMI *before* writing to MSR counter when we get event notification.

Signed-off-by Eric Dumazet <[email protected]>

arch/x86/oprofile/op_model_ppro.c | 8 +++++---

1 files changed, 5 insertions(+), 3 deletions(-)
arch/x86/oprofile/op_model_ppro.c +5 -3
index 3f1b81a..7b142da 100644
@@ -132,13 +132,15 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 		rdmsrl(msrs->counters[i].addr, val);
 		if (CTR_OVERFLOWED(val)) {
 			oprofile_add_sample(regs, i);
+			/*
+			 * We need to unmask the apic vector *before*
+			 * writing reset_value to msr counter
+			 */
+			apic_write(APIC_LVTPC, APIC_DM_NMI);
 			wrmsrl(msrs->counters[i].addr, -reset_value[i]);
 		}
 	}
 
-	/* Only P6 based Pentium M need to re-unmask the apic vector but it
-	 * doesn't hurt other P6 variant */
-	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
 
 	/* We can't work out if we really handled an interrupt. We
 	 * might have caught a *second* counter just after overflowing
#6 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

arch/x86/oprofile/op_model_ppro.c static int ppro_check_ctrs(struct pt_regs * const regs, · line 141

Quoted 13 lines
arch/x86/oprofile/op_model_ppro.c +5 -3
 			oprofile_add_sample(regs, i);
+			/*
+			 * We need to unmask the apic vector *before*
+			 * writing reset_value to msr counter
+			 */
+			apic_write(APIC_LVTPC, APIC_DM_NMI);
 			wrmsrl(msrs->counters[i].addr, -reset_value[i]);
 		}
 	}
 
-	/* Only P6 based Pentium M need to re-unmask the apic vector but it
-	 * doesn't hurt other P6 variant */
-	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);

Did you also test if it really needs to be inside the if () or just before the wrmsrl?

-Andi

#7 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

Quoted 10 lines · Andi Kleen
Quoted 10 lines
arch/x86/oprofile/op_model_ppro.c +5 -0
 			oprofile_add_sample(regs, i);
+			/*
+			 * We need to unmask the apic vector *before*
+			 * writing reset_value to msr counter
+			 */
+			apic_write(APIC_LVTPC, APIC_DM_NMI);
 			wrmsrl(msrs->counters[i].addr, -reset_value[i]);
 		}
 	}
Quoted 5 lines

arch/x86/oprofile/op_model_ppro.c static int ppro_check_ctrs(struct pt_regs * const regs, · line 141

Quoted 3 lines
arch/x86/oprofile/op_model_ppro.c +0 -3
-	/* Only P6 based Pentium M need to re-unmask the apic vector but it
-	 * doesn't hurt other P6 variant */
-	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);

Did you also test if it really needs to be inside the if () or just before the wrmsrl?

In my testings, the "if (CTR_OVERFLOWED())" condition is always true.

(I am profiling one event only)

I felt uncomfortable issuing the apic_write() before oprofile_add_sample()

Do you mean doing the apic_write() right at the beginning of the ppro_check_ctrs() function ?

#8 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

Quoted 1 line · Eric Dumazet

Andi Kleen a écrit

Can you send the /proc/cpuinfo output privately please? I can ask around here.

Quoted 18 lines
Quoted 10 lines
Quoted 10 lines
			oprofile_add_sample(regs, i);
arch/x86/oprofile/op_model_ppro.c +5 -0
+			/*
+			 * We need to unmask the apic vector *before*
+			 * writing reset_value to msr counter
+			 */
+			apic_write(APIC_LVTPC, APIC_DM_NMI);

			wrmsrl(msrs->counters[i].addr, -reset_value[i]);
		}
	}
Quoted 6 lines

arch/x86/oprofile/op_model_ppro.c static int ppro_check_ctrs(struct pt_regs * const regs, · line 141

Quoted 3 lines
arch/x86/oprofile/op_model_ppro.c +0 -3
-	/* Only P6 based Pentium M need to re-unmask the apic vector but it
-	 * doesn't hurt other P6 variant */
-	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);

Did you also test if it really needs to be inside the if () or just before the wrmsrl?

Quoted 3 lines

In my testings, the "if (CTR_OVERFLOWED())" condition is always true.

(I am profiling one event only)

There can be other valid (but rare) cases of NMIs. The code handles this by not having any overflow events.

It might be that the case where your NMI gets stuck is such a stray NMI?

That is in theory it should be safe to unmask that vector multiple times (the other NMIs should come from other vectors), but local APIC is an fragile area.

Quoted 6 lines

I felt uncomfortable issuing the apic_write() before oprofile_add_sample()

Do you mean doing the apic_write() right at the beginning of the ppro_check_ctrs()
function ?

Better once, otherwise you'll do it multiple times worst case (multiple counters can overflow)

-Andi

Signature · 1 line
[email protected]
#9 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

In reply to Andi Kleen
Quoted 10 lines · Andi Kleen
Quoted 10 lines
(quoted diff) +5 -0
 			oprofile_add_sample(regs, i);
+			/*
+			 * We need to unmask the apic vector *before*
+			 * writing reset_value to msr counter
+			 */
+			apic_write(APIC_LVTPC, APIC_DM_NMI);
 			wrmsrl(msrs->counters[i].addr, -reset_value[i]);
 		}
 	}
Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +0 -3
-	/* Only P6 based Pentium M need to re-unmask the apic vector but it
-	 * doesn't hurt other P6 variant */
-	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);

Did you also test if it really needs to be inside the if () or just before the wrmsrl?

Andi, the following patch also works well for me. The key is we must unmask APIC before the wrmsrl. after/before the rdmsrl has no impact.

I wonder then if the final comment in ppro_check_ctrs() is still applicable.

PATCH] oprofile: un-mask APIC before resetting counter in ppro_check_ctrs()

While using oprofile on my HP BL460c G1, (two quad core intel E5450 CPU), I noticed that one CPU after the other could not get anymore NMI.

After a while, all cores where blocked (ie not generating events for oprofile) I tried all major linux versions and all where affected by this freeze.

I found that we have to un-mask APIC *before* writing to MSR counter when we get event notification, because we use APIC_LVTPC in edge triggered mode.

Signed-off-by Eric Dumazet <[email protected]>

---
arch/x86/oprofile/op_model_ppro.c | 10 ++++++----

1 files changed, 6 insertions(+), 4 deletions(-)
arch/x86/oprofile/op_model_ppro.c +6 -4
index 3f1b81a..8484528 100644
@@ -126,6 +126,12 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 	u64 val;
 	int i;
 
+	/*
+	 * We need to unmask the apic vector *before* writing reset_value
+	 * to msr counter, because we use edge trigger
+	 */
+	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
+
 	for (i = 0 ; i < num_counters; ++i) {
 		if (!reset_value[i])
 			continue;

@@ -136,10 +142,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 		}
 	}
 
-	/* Only P6 based Pentium M need to re-unmask the apic vector but it
-	 * doesn't hurt other P6 variant */
-	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
-
 	/* We can't work out if we really handled an interrupt. We
 	 * might have caught a *second* counter just after overflowing
 	 * the interrupt for this counter then arrives
author
#10 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

On 11.11.08 09:32:12, Eric Dumazet wrote:

[...]

Quoted 1 line

PATCH] oprofile: un-mask APIC before resetting counter in ppro_check_ctrs()

Is this the patch you all prefer? If so, I would like to send it upstream.

-Robert

Quoted 45 lines

While using oprofile on my HP BL460c G1, (two quad core intel E5450 CPU), I noticed that one CPU after the other could not get anymore NMI.

After a while, all cores where blocked (ie not generating events for oprofile)
I tried all major linux versions and all where affected by this freeze.

I found that we have to un-mask APIC *before* writing to MSR counter when we get event notification, because we use APIC_LVTPC in edge triggered mode.

Signed-off-by Eric Dumazet <[email protected]>

---
arch/x86/oprofile/op_model_ppro.c | 10 ++++++----

1 files changed, 6 insertions(+), 4 deletions(-)
arch/x86/oprofile/op_model_ppro.c +6 -4
index 3f1b81a..8484528 100644
@@ -126,6 +126,12 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 	u64 val;
 	int i;
 
+	/*
+	 * We need to unmask the apic vector *before* writing reset_value
+	 * to msr counter, because we use edge trigger
+	 */
+	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
+
 	for (i = 0 ; i < num_counters; ++i) {
 		if (!reset_value[i])
 			continue;

@@ -136,10 +142,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 		}
 	}
 
-	/* Only P6 based Pentium M need to re-unmask the apic vector but it
-	 * doesn't hurt other P6 variant */
-	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
-
 	/* We can't work out if we really handled an interrupt. We
 	 * might have caught a *second* counter just after overflowing
 	 * the interrupt for this counter then arrives
Signature · 3 lines
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]
#11 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

Quoted 8 lines · Robert Richter

On 11.11.08 09:32:12, Eric Dumazet wrote:

[...]

Quoted 1 line

PATCH] oprofile: un-mask APIC before resetting counter in ppro_check_ctrs()

Is this the patch you all prefer? If so, I would like to send it upstream.

It would be better to root cause the problem first.

I worked with Eric privately and tried a few things, but didn't help so far. Most likely the workaround is also not complete before it is fully understood. Not sure how long this will take though.

It's probably ok as a workaround for a release, but shouldn't be considered a final fix.

-Andi

author
#12 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

Quoted 4 lines · Andi Kleen
Quoted 2 lines · Robert Richter

Is this the patch you all prefer? If so, I would like to send it upstream.

Quoted 8 lines

It would be better to root cause the problem first.

I worked with Eric privately and tried a few things, but didn't help so far. Most likely the workaround is also not complete before it is fully understood. Not sure how long this will take though.

It's probably ok as a workaround for a release, but shouldn't be considered a final fix.

Ok, I will take no action. Let me know if there is a new solution available or if this workaround should go in.

-Robert

Signature · 3 lines
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]
#13 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

In reply to Eric Dumazet

[Eric Dumazet - Mon, Nov 10, 2008 at 03:23:00PM +0100]

Quoted 44 lines
Quoted 14 lines · Andi Kleen

arch/x86/oprofile/op_model_ppro.c static void ppro_setup_ctrs(struct op_msrs const * const msrs) · line 71

Quoted 9 lines · Eric Dumazet
arch/x86/oprofile/op_model_ppro.c +1 -1
index 3f1b81a..716d26f 100644
@@ -69,7 +69,7 @@ static void ppro_setup_ctrs(struct op_msrs const * const msrs)
 	int i;
  	if (!reset_value) {
-		reset_value = kmalloc(sizeof(unsigned) * num_counters,
+		reset_value = kmalloc(sizeof(reset_value[0]) * num_counters,

Thanks for tracking this down.

Quoted 1 line

But that still doesn't explain why 2.6.27 fails too?

Desesperatly Seeking Oprofile, next round.

I know *nothing* about APIC but spent few hours to try several tricks and finally found something.

It solved my problem : oprofile can run several hours without any freeze of NMI on any core.

# grep NMI /proc/interrupts
NMI:   10902884    9635871   10333815    8372989    7971483    8298373    8877495   10206963   Non-maskable interrupts
...
# grep NMI /proc/interrupts
NMI:   15518834   14340713   15038694   13078235   12676585   13003394   13582115   14912146   Non-maskable interrupts

Can anybody understand and explain what is happening ?

Is it a software or hardware problem ?

[PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

While using oprofile on my HP BL460c G1, (two quad core intel E5450 CPU), I noticed that one CPU after the other could not get anymore NMI.

After a while, all cores where blocked (ie not generating events for oprofile) I tried all major linux versions and all where affected by this freeze.

Quoted 6 lines

I found that we have to re-arm APIC_DM_NMI *before* writing to MSR counter when we get event notification.

Signed-off-by Eric Dumazet <[email protected]>

arch/x86/oprofile/op_model_ppro.c | 8 +++++---

1 files changed, 5 insertions(+), 3 deletions(-)
| diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
| index 3f1b81a..7b142da 100644
| --- a/arch/x86/oprofile/op_model_ppro.c
| +++ b/arch/x86/oprofile/op_model_ppro.c
| @@ -132,13 +132,15 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
|  		rdmsrl(msrs->counters[i].addr, val);
|  		if (CTR_OVERFLOWED(val)) {
|  			oprofile_add_sample(regs, i);
| +			/*
| +			 * We need to unmask the apic vector *before*
| +			 * writing reset_value to msr counter
| +			 */
| +			apic_write(APIC_LVTPC, APIC_DM_NMI);
|  			wrmsrl(msrs->counters[i].addr, -reset_value[i]);
|  		}
|  	}
|  
| -	/* Only P6 based Pentium M need to re-unmask the apic vector but it
| -	 * doesn't hurt other P6 variant */
| -	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
|  
|  	/* We can't work out if we really handled an interrupt. We
|  	 * might have caught a *second* counter just after overflowing

Hi Eric,

for the record

	apic_write(APIC_LVTPC, APIC_DM_NMI);

is not just 'unmask' but also *zeroify* (not sure if I wrote this word right :) all fields when the origianl code was just 'unmasking' TPC register

	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);

that is why apic_read() was in former.

- Cyrill -

#14 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

Quoted 78 lines · Cyrill Gorcunov

[Eric Dumazet - Mon, Nov 10, 2008 at 03:23:00PM +0100]

Quoted 42 lines
Quoted 13 lines · Andi Kleen

arch/x86/oprofile/op_model_ppro.c static void ppro_setup_ctrs(struct op_msrs const * const msrs) · line 71

Quoted 9 lines · Eric Dumazet
arch/x86/oprofile/op_model_ppro.c +1 -1
index 3f1b81a..716d26f 100644
@@ -69,7 +69,7 @@ static void ppro_setup_ctrs(struct op_msrs const * const msrs)
 	int i;
  	if (!reset_value) {
-		reset_value = kmalloc(sizeof(unsigned) * num_counters,
+		reset_value = kmalloc(sizeof(reset_value[0]) * num_counters,

Thanks for tracking this down.

Quoted 1 line

But that still doesn't explain why 2.6.27 fails too?

Desesperatly Seeking Oprofile, next round.

I know *nothing* about APIC but spent few hours to try several tricks and finally found something.

It solved my problem : oprofile can run several hours without any freeze of NMI on any core.

# grep NMI /proc/interrupts
NMI:   10902884    9635871   10333815    8372989    7971483    8298373    8877495   10206963   Non-maskable interrupts
...
# grep NMI /proc/interrupts
NMI:   15518834   14340713   15038694   13078235   12676585   13003394   13582115   14912146   Non-maskable interrupts

Can anybody understand and explain what is happening ?

Is it a software or hardware problem ?

[PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

While using oprofile on my HP BL460c G1, (two quad core intel E5450 CPU), I noticed that one CPU after the other could not get anymore NMI.

After a while, all cores where blocked (ie not generating events for oprofile) I tried all major linux versions and all where affected by this freeze.

Quoted 6 lines

I found that we have to re-arm APIC_DM_NMI *before* writing to MSR counter when we get event notification.

Signed-off-by Eric Dumazet <[email protected]>

arch/x86/oprofile/op_model_ppro.c | 8 +++++---

1 files changed, 5 insertions(+), 3 deletions(-)
| diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
| index 3f1b81a..7b142da 100644
| --- a/arch/x86/oprofile/op_model_ppro.c
| +++ b/arch/x86/oprofile/op_model_ppro.c
| @@ -132,13 +132,15 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
|  		rdmsrl(msrs->counters[i].addr, val);
|  		if (CTR_OVERFLOWED(val)) {
|  			oprofile_add_sample(regs, i);
| +			/*
| +			 * We need to unmask the apic vector *before*
| +			 * writing reset_value to msr counter
| +			 */
| +			apic_write(APIC_LVTPC, APIC_DM_NMI);
|  			wrmsrl(msrs->counters[i].addr, -reset_value[i]);
|  		}
|  	}
|  | -	/* Only P6 based Pentium M need to re-unmask the apic vector but it
| -	 * doesn't hurt other P6 variant */
| -	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
|  |  	/* We can't work out if we really handled an interrupt. We
|  	 * might have caught a *second* counter just after overflowing

Hi Eric,

for the record

	apic_write(APIC_LVTPC, APIC_DM_NMI);
Quoted 8 lines

is not just 'unmask' but also *zeroify* (not sure if I wrote this word right :) all fields when the origianl code was just 'unmasking' TPC register

	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);

that is why apic_read() was in former.

Well, given that APIC_LVTPC is initialized by oprofile init to value APIC_DM_NMI, I avoid an apic_read() and just write APIC_DM_NMI again...

Presumably, apic_read(APIC_LVTPC) should return APIC_DM_NMI or APIC_DM_NMI|APIC_LVT_MASKED

Thanks

#15 lore raw

Re: [PATCH] oprofile: re-arm APIC_DM_NMI in ppro_check_ctrs()

[Eric Dumazet - Mon, Nov 10, 2008 at 05:19:20PM +0100]
...

Quoted 16 lines
Quoted 7 lines

Hi Eric,

for the record

	apic_write(APIC_LVTPC, APIC_DM_NMI);
Quoted 8 lines

is not just 'unmask' but also *zeroify* (not sure if I wrote this word right :) all fields when the origianl code was just 'unmasking' TPC register

	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);

that is why apic_read() was in former.

Quoted 7 lines

Well, given that APIC_LVTPC is initialized by oprofile init to value APIC_DM_NMI, I avoid an apic_read() and just write APIC_DM_NMI again...

Presumably, apic_read(APIC_LVTPC) should return APIC_DM_NMI or APIC_DM_NMI|APIC_LVT_MASKED

Thanks

Yes, just grepped the sources -- it seems nobody else touching this register indeed.
- Cyrill -

author Applied
#16 lore raw
In reply to Eric Dumazet
Quoted 9 lines · Eric Dumazet

[PATCH] oprofile: fix an overflow in ppro code

reset_value was changed from long to u64 in commit b99170288421c79f0c2efa8b33e26e65f4bb7fb8 (oprofile: Implement Intel architectural perfmon support)

But dynamic allocation of this array use a wrong type (long instead of u64)

Signed-off-by Eric Dumazet <[email protected]>

Applied to oprofile/oprofile-for-tip. Thanks Eric.

-Robert

Signature · 3 lines
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]