[PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Cyrill Gorcunov]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Don, I've added yours SOB, ok? (The patch is attached to avoid
space/tabs problem
due to web-mail client)

[-- Attachment #2: perf-x86-p4-pmu-unflagged --]
[-- Type: application/octet-stream, Size: 896 bytes --]

From: Don Zickus <dzickus@redhat.com>
Subject: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

The read of real MSR register was missed before
if () clause which leads that test never success.

Signed-off-by: Don Zickus <dzickus@redhat.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/kernel/cpu/perf_event_p4.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.tip/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================
--- linux-2.6.tip.orig/arch/x86/kernel/cpu/perf_event_p4.c
+++ linux-2.6.tip/arch/x86/kernel/cpu/perf_event_p4.c
@@ -777,6 +777,7 @@ static inline int p4_pmu_clear_cccr_ovf(
 	 * the counter has reached zero value and continued counting before
 	 * real NMI signal was received:
 	 */
+	rdmsrl(hwc->event_base, v);
 	if (!(v & ARCH_P4_UNFLAGGED_BIT))
 		return 1;
 

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Don, I've added yours SOB, ok? (The patch is attached to avoid
> space/tabs problem
> due to web-mail client)

The patch lacks a proper description about the motivation and effects of the 
patch.

Thanks,

	Ingo

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Cyrill Gorcunov]

On Thursday, March 24, 2011, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> Don, I've added yours SOB, ok? (The patch is attached to avoid
>> space/tabs problem
>> due to web-mail client)
>
> The patch lacks a proper description about the motivation and effects of the
> patch.
>
> Thanks,
>
>         Ingo
>

Ok, i'll resend more detailed description later, thanks.

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Don Zickus]

On Thu, Mar 24, 2011 at 11:29:43AM +0300, Cyrill Gorcunov wrote:
> Don, I've added yours SOB, ok? (The patch is attached to avoid
> space/tabs problem
> due to web-mail client)

Fine by me.

Cheers,
Don

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Cyrill Gorcunov]

On 03/24/2011 11:48 AM, Ingo Molnar wrote:
> 
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
>> Don, I've added yours SOB, ok? (The patch is attached to avoid
>> space/tabs problem
>> due to web-mail client)
> 
> The patch lacks a proper description about the motivation and effects of the 
> patch.
> 
> Thanks,
> 
> 	Ingo

Ingo, does this one looks better?

---
From: Don Zickus <dzickus@redhat.com>
Subject: [PATCH -tip] perf, x86: P4 PMU - Add missing read of MSR register to catch unflagged overflows

The read of a proper MSR register was missed so instead of a counter the
configration register is tested (it has ARCH_P4_UNFLAGGED_BIT always
cleared) and unflagged overflows never have been catched. Fix it by
reading a proper MSR register.

Signed-off-by: Don Zickus <dzickus@redhat.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/kernel/cpu/perf_event_p4.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.tip/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================
--- linux-2.6.tip.orig/arch/x86/kernel/cpu/perf_event_p4.c
+++ linux-2.6.tip/arch/x86/kernel/cpu/perf_event_p4.c
@@ -777,6 +777,7 @@ static inline int p4_pmu_clear_cccr_ovf(
 	 * the counter has reached zero value and continued counting before
 	 * real NMI signal was received:
 	 */
+	rdmsrl(hwc->event_base, v);
 	if (!(v & ARCH_P4_UNFLAGGED_BIT))
 		return 1;


-- 
    Cyrill

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On 03/24/2011 11:48 AM, Ingo Molnar wrote:
> > 
> > * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > 
> >> Don, I've added yours SOB, ok? (The patch is attached to avoid
> >> space/tabs problem
> >> due to web-mail client)
> > 
> > The patch lacks a proper description about the motivation and effects of the 
> > patch.
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> Ingo, does this one looks better?
> 
> ---
> From: Don Zickus <dzickus@redhat.com>
> Subject: [PATCH -tip] perf, x86: P4 PMU - Add missing read of MSR register to catch unflagged overflows
> 
> The read of a proper MSR register was missed so instead of a counter the 
> configration register is tested (it has ARCH_P4_UNFLAGGED_BIT always cleared) 
> and unflagged overflows never have been catched. Fix it by reading a proper 
> MSR register.

So what effect does this have on the regular perf user? Please try to describe 
the real-life effect of the bug/problem fixed here.

Thanks,

	Ingo

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Cyrill Gorcunov]

On 03/24/2011 07:33 PM, Ingo Molnar wrote:
> 
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
>> On 03/24/2011 11:48 AM, Ingo Molnar wrote:
>>>
>>> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>>
>>>> Don, I've added yours SOB, ok? (The patch is attached to avoid
>>>> space/tabs problem
>>>> due to web-mail client)
>>>
>>> The patch lacks a proper description about the motivation and effects of the 
>>> patch.
>>>
>>> Thanks,
>>>
>>> 	Ingo
>>
>> Ingo, does this one looks better?
>>
>> ---
>> From: Don Zickus <dzickus@redhat.com>
>> Subject: [PATCH -tip] perf, x86: P4 PMU - Add missing read of MSR register to catch unflagged overflows
>>
>> The read of a proper MSR register was missed so instead of a counter the 
>> configration register is tested (it has ARCH_P4_UNFLAGGED_BIT always cleared) 
>> and unflagged overflows never have been catched. Fix it by reading a proper 
>> MSR register.
> 
> So what effect does this have on the regular perf user? Please try to describe 
> the real-life effect of the bug/problem fixed here.
> 
> Thanks,
> 
> 	Ingo

Unflagged overflows never have been catched due to missed read of a register which
is to signalize about it, and as result unknown nmi may happen leading to
"Dazen and confused" message. That is what supposed to be in changelog?

-- 
    Cyrill

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Cyrill Gorcunov]

On 03/24/2011 07:46 PM, Cyrill Gorcunov wrote:
...
>>
>> So what effect does this have on the regular perf user? Please try to describe 
>> the real-life effect of the bug/problem fixed here.
>>
>> Thanks,
>>
>> 	Ingo
> 
> Unflagged overflows never have been catched due to missed read of a register which
> is to signalize about it, and as result unknown nmi may happen leading to
> "Dazen and confused" message. That is what supposed to be in changelog?
> 

Or you mean the tech details would be appropriate in changelog as well? If so
I could put such details there.

-- 
    Cyrill

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Unflagged overflows never have been catched due to missed read of a register which
> is to signalize about it, and as result unknown nmi may happen leading to
> "Dazen and confused" message. That is what supposed to be in changelog?

Exactly, the 'Dazed and confused' message is *all* that the user cares about so 
it must feature prominently in the changelog.

If a P4 user searches lkml he wants to know which fixed address 
dazed-and-confused messages. He will know nothing about 'unflagged overflows' 
or other internals ...

All the other details about how the patch does the fix is secondary to what 
users experience when they hit this bug.

Thanks,

	Ingo

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Cyrill Gorcunov]

On 03/24/2011 07:51 PM, Ingo Molnar wrote:
> 
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
>> Unflagged overflows never have been catched due to missed read of a register which
>> is to signalize about it, and as result unknown nmi may happen leading to
>> "Dazen and confused" message. That is what supposed to be in changelog?
> 
> Exactly, the 'Dazed and confused' message is *all* that the user cares about so 
> it must feature prominently in the changelog.
> 
> If a P4 user searches lkml he wants to know which fixed address 
> dazed-and-confused messages. He will know nothing about 'unflagged overflows' 
> or other internals ...
> 
> All the other details about how the patch does the fix is secondary to what 
> users experience when they hit this bug.
> 
> Thanks,
> 
> 	Ingo

ok, let me try
---
From: Don Zickus <dzickus@redhat.com>
Subject: [PATCH -tip] perf, x86: P4 PMU - Catch unknown NMI on unflagged overflows

The read of a proper MSR register was missed and instead of counter the
configration register was tested (it has ARCH_P4_UNFLAGGED_BIT always
cleared) leading to unknown NMI hitting the system. As result the user may
obtain "Dazed and confused, but trying to continue" message. Fix it by reading
a proper MSR register.

Signed-off-by: Don Zickus <dzickus@redhat.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/kernel/cpu/perf_event_p4.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.tip/arch/x86/kernel/cpu/perf_event_p4.c
===================================================================
--- linux-2.6.tip.orig/arch/x86/kernel/cpu/perf_event_p4.c
+++ linux-2.6.tip/arch/x86/kernel/cpu/perf_event_p4.c
@@ -777,6 +777,7 @@ static inline int p4_pmu_clear_cccr_ovf(
 	 * the counter has reached zero value and continued counting before
 	 * real NMI signal was received:
 	 */
+	rdmsrl(hwc->event_base, v);
 	if (!(v & ARCH_P4_UNFLAGGED_BIT))
 		return 1;

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Don Zickus]

On Thu, Mar 24, 2011 at 07:46:40PM +0300, Cyrill Gorcunov wrote:
> >> The read of a proper MSR register was missed so instead of a counter the 
> >> configration register is tested (it has ARCH_P4_UNFLAGGED_BIT always cleared) 
> >> and unflagged overflows never have been catched. Fix it by reading a proper 
> >> MSR register.
> > 
> > So what effect does this have on the regular perf user? Please try to describe 
> > the real-life effect of the bug/problem fixed here.
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> Unflagged overflows never have been catched due to missed read of a register which
> is to signalize about it, and as result unknown nmi may happen leading to
> "Dazen and confused" message. That is what supposed to be in changelog?

I think Ingo is looking for something like this:

When an NMI happens on a P4, the perf nmi handler checks the configuration
register to see if the overflow bit is set or not before taking
appropriate action.  Unfortunately, various P4 machines had a broken
overflow bit, so a backup mechanism was implemented.  This mechanism
checked to see if the counter rolled over or not.

A previous commit that implemented this backup mechanism was broken.
Instead of reading the counter register, it used the configuration
register to determine if the counter rolled over or not.  Reading that bit
would give incorrect results.

This would lead to 'Dazed and confused' messages for the end user when
using the perf tool (or if the nmi watchdog is running).

The fix is to read the counter register before determining if the counter
rolled over or not.

Cheers,
Don

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Cyrill Gorcunov]

On 03/24/2011 09:22 PM, Don Zickus wrote:
> On Thu, Mar 24, 2011 at 07:46:40PM +0300, Cyrill Gorcunov wrote:
>>>> The read of a proper MSR register was missed so instead of a counter the 
>>>> configration register is tested (it has ARCH_P4_UNFLAGGED_BIT always cleared) 
>>>> and unflagged overflows never have been catched. Fix it by reading a proper 
>>>> MSR register.
>>>
>>> So what effect does this have on the regular perf user? Please try to describe 
>>> the real-life effect of the bug/problem fixed here.
>>>
>>> Thanks,
>>>
>>> 	Ingo
>>
>> Unflagged overflows never have been catched due to missed read of a register which
>> is to signalize about it, and as result unknown nmi may happen leading to
>> "Dazen and confused" message. That is what supposed to be in changelog?
> 
> I think Ingo is looking for something like this:
> 

Thanks Don, if Ingo agree I can update it.

-- 
    Cyrill

Re: [PATCH -tip] perf, x86: P4 PMU - Add missing read of a counter before test

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On 03/24/2011 09:22 PM, Don Zickus wrote:
> > On Thu, Mar 24, 2011 at 07:46:40PM +0300, Cyrill Gorcunov wrote:
> >>>> The read of a proper MSR register was missed so instead of a counter the 
> >>>> configration register is tested (it has ARCH_P4_UNFLAGGED_BIT always cleared) 
> >>>> and unflagged overflows never have been catched. Fix it by reading a proper 
> >>>> MSR register.
> >>>
> >>> So what effect does this have on the regular perf user? Please try to describe 
> >>> the real-life effect of the bug/problem fixed here.
> >>>
> >>> Thanks,
> >>>
> >>> 	Ingo
> >>
> >> Unflagged overflows never have been catched due to missed read of a register which
> >> is to signalize about it, and as result unknown nmi may happen leading to
> >> "Dazen and confused" message. That is what supposed to be in changelog?
> > 
> > I think Ingo is looking for something like this:
> > 
> 
> Thanks Don, if Ingo agree I can update it.

Sure - please resend the final patch in a clean thread, with a proper title, 
etc.

Thanks,

	Ingo