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"
--
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]
* Robert Richter <[email protected]> wrote:
> 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
diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
index 3f1b81a..716d26f 100644
--- a/arch/x86/oprofile/op_model_ppro.c
+++ b/arch/x86/oprofile/op_model_ppro.c
@@ -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;
Eric Dumazet <[email protected]> writes:
> diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
> index 3f1b81a..716d26f 100644
> --- a/arch/x86/oprofile/op_model_ppro.c
> +++ b/arch/x86/oprofile/op_model_ppro.c
> @@ -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
--
[email protected]
Andi Kleen a ?crit :
> Eric Dumazet <[email protected]> writes:
>
>> diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
>> index 3f1b81a..716d26f 100644
>> --- a/arch/x86/oprofile/op_model_ppro.c
>> +++ b/arch/x86/oprofile/op_model_ppro.c
>> @@ -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?
No :(
In fact 2.6.24 fails too. Oprofile never worked correctly.
Still investigating...
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
> 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
Andi Kleen a ?crit :
>> 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?
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 ?
[Eric Dumazet - Mon, Nov 10, 2008 at 03:23:00PM +0100]
> Andi Kleen a ?crit :
>> Eric Dumazet <[email protected]> writes:
>>
>>> diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
>>> index 3f1b81a..716d26f 100644
>>> --- a/arch/x86/oprofile/op_model_ppro.c
>>> +++ b/arch/x86/oprofile/op_model_ppro.c
>>> @@ -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?
>
> 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(-)
| 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 -
Cyrill Gorcunov a ?crit :
> [Eric Dumazet - Mon, Nov 10, 2008 at 03:23:00PM +0100]
>> Andi Kleen a ?crit :
>>> Eric Dumazet <[email protected]> writes:
>>>
>>>> diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
>>>> index 3f1b81a..716d26f 100644
>>>> --- a/arch/x86/oprofile/op_model_ppro.c
>>>> +++ b/arch/x86/oprofile/op_model_ppro.c
>>>> @@ -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?
>> 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(-)
>
> | 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.
>
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
[Eric Dumazet - Mon, Nov 10, 2008 at 05:19:20PM +0100]
...
>>
>> 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.
>>
>
> 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 -
On Mon, Nov 10, 2008 at 04:50:00PM +0100, Eric Dumazet wrote:
> Andi Kleen a ?crit
Can you send the /proc/cpuinfo output privately please?
I can ask around here.
> >> 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?
>
>
> 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.
>
> 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
--
[email protected]
diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
index 3f1b81a..8484528 100644
--- a/arch/x86/oprofile/op_model_ppro.c
+++ b/arch/x86/oprofile/op_model_ppro.c
@@ -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
On 11.11.08 09:32:12, Eric Dumazet wrote:
[...]
> 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
>
> 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(-)
> diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
> index 3f1b81a..8484528 100644
> --- a/arch/x86/oprofile/op_model_ppro.c
> +++ b/arch/x86/oprofile/op_model_ppro.c
> @@ -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
--
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]
On 10.11.08 09:05:37, Eric Dumazet wrote:
> [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
--
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]
On Mon, Nov 17, 2008 at 06:33:59PM +0100, Robert Richter wrote:
> On 11.11.08 09:32:12, Eric Dumazet wrote:
>
> [...]
>
> > 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
On 17.11.08 19:25:27, Andi Kleen wrote:
> On Mon, Nov 17, 2008 at 06:33:59PM +0100, Robert Richter wrote:
> > 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.
Ok, I will take no action. Let me know if there is a new solution
available or if this workaround should go in.
-Robert
--
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]