updates for oprofile

GIT PULL New Phil Carmody 3 participants 8 messages Apr 27, 2010 – May 3, 2010

lore

Share & tools

lore

Phil Carmody

author

2010-04-27 15:26 UTC #0 lore raw

Ingo, et al.,

Regarding today's pulled request, containing:

commit bc078e4eab65f11bbaeed380593ab8151b30d703

Author: Martin Schwidefsky <[email protected]>
Date:   Tue Mar 2 16:01:10 2010 +0100

oprofile: convert oprofile from timer_hook to hrtimer

Information is a touch scant, as I'm doing the investigation as I write, but I believe that that patch can cause ooops regressions via a null-pointer dereference in oprofile_add_sample().

That function declares:
"""
/**
* Add a sample. This may be called from any context.
*/
void oprofile_add_sample(struct pt_regs * const regs, unsigned long event); """

And begins:
"""
void oprofile_add_sample(struct pt_regs * const regs, unsigned long event)

{
        int is_kernel = !user_mode(regs);

"""

Where on at least two major architectures (Arm, x86), user_mode() unconditionally dereferences its parameter.

Now oprofile_add_sample() is called from this context:
"""
static enum hrtimer_restart oprofile_hrtimer_notify(struct hrtimer *hrtimer)

{
         oprofile_add_sample(get_irq_regs(), 0);

"""

And get_irq_regs() is NULL when not in an IRQ context.

Bang.

An example of this kind of thing kicking in has already been encountered last year:
http://www.mail-archive.com/[email protected]/msg14069.html
(That thread got a little side-tracked onto OMAP specifics, but the original report is topical.)

Now would be a very good time for the "many eyes" principle to kick in.

I'm now looking into workarounds, but nothing that I'd necessarily want to submit as a real fix.

Phil
cc:'d replies appreciated

Robert Richter

2010-04-27 17:40 UTC #1 lore raw

(cc'ing oprofile-list)

Quoted 33 lines · Phil Carmody

Ingo, et al.,
Regarding today's pulled request, containing:
commit bc078e4eab65f11bbaeed380593ab8151b30d703
Author: Martin Schwidefsky <[email protected]>
Date:   Tue Mar 2 16:01:10 2010 +0100
oprofile: convert oprofile from timer_hook to hrtimer
Information is a touch scant, as I'm doing the investigation as I write, but I believe that that patch can cause ooops regressions via a null-pointer dereference in oprofile_add_sample().
That function declares:
"""
/**
* Add a sample. This may be called from any context.
*/
void oprofile_add_sample(struct pt_regs * const regs, unsigned long event); """
And begins:
"""
void oprofile_add_sample(struct pt_regs * const regs, unsigned long event)
{
        int is_kernel = !user_mode(regs);
"""
Where on at least two major architectures (Arm, x86), user_mode() unconditionally dereferences its parameter.

Quoted 8 lines

Now oprofile_add_sample() is called from this context:
"""
static enum hrtimer_restart oprofile_hrtimer_notify(struct hrtimer *hrtimer)
{
         oprofile_add_sample(get_irq_regs(), 0);
"""
And get_irq_regs() is NULL when not in an IRQ context.

Perf is simply dropping the sample in such cases, see:

kernel/perf_event.c:perf_swevent_hrtimer()

So at quick fix would be to check for a null pointer also. But, according to this:

http://www.mail-archive.com/[email protected]/msg14074.html

samples will be incorrect then since only interrupt context is profiled. It seems there is no solution available right now.

-Robert

Quoted 17 lines

Bang.
An example of this kind of thing kicking in has already been encountered last year:
http://www.mail-archive.com/[email protected]/msg14069.html
(That thread got a little side-tracked onto OMAP specifics, but the original report is topical.)
Now would be a very good time for the "many eyes" principle to kick in.
I'm now looking into workarounds, but nothing that I'd necessarily want to submit as a real fix.
Phil
cc:'d replies appreciated

Signature · 3 lines

Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]

Siarhei Siamashka

2010-04-27 17:50 UTC #2 lore raw

Quoted 14 lines · On Tuesday 27 April 2010 20:40:26 ext Robert Richter wrote:

Quoted 8 lines · Phil Carmody
Now oprofile_add_sample() is called from this context:
"""
static enum hrtimer_restart oprofile_hrtimer_notify(struct hrtimer
*hrtimer) {
         oprofile_add_sample(get_irq_regs(), 0);
"""
And get_irq_regs() is NULL when not in an IRQ context.
Perf is simply dropping the sample in such cases, see:
kernel/perf_event.c:perf_swevent_hrtimer()

Quoted 7 lines

So at quick fix would be to check for a null pointer also. But, according to this:
http://www.mail-archive.com/[email protected]/msg14074.html
samples will be incorrect then since only interrupt context is profiled. It seems there is no solution available right now.

Isn't hrtimer callback function supposed to be only called from IRQ context after this cleanup: http://lwn.net/Articles/308545/ ?

Signature · 2 lines

Best regards,
Siarhei Siamashka

Robert Richter

2010-04-28 16:59 UTC #3 lore raw

Quoted 2 lines · Siarhei Siamashka

Isn't hrtimer callback function supposed to be only called from IRQ context after this cleanup: http://lwn.net/Articles/308545/ ?

Yes, the patch is upstream since v2.6.29. Thanks Siarhei.

I will add a null pointer check anyway.

-Robert

Signature · 3 lines

Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]

Phil Carmody

author

2010-04-28 17:09 UTC #4 lore raw

Quoted 7 lines · ext Robert Richter

Quoted 2 lines · Siarhei Siamashka
Isn't hrtimer callback function supposed to be only called from IRQ context after this cleanup: http://lwn.net/Articles/308545/ ?
Yes, the patch is upstream since v2.6.29. Thanks Siarhei.
I will add a null pointer check anyway.

A few here thrashed around a couple of ideas, and the general consensus was that the following work for us, and is offered for consideration.

Phil

From: Phil Carmody <[email protected]>
Date: Tue, 27 Apr 2010 19:28:33 +0300
Subject: [PATCH v2 1/1] oprofile: HACK - protect from not being in an IRQ context

http://lkml.org/lkml/2010/4/27/285

Protect against dereferencing regs when it's NULL, and force a magic number into pc to prevent too deep processing. This approach permits the dropped samples to be tallied as invalid Instruction Pointer events.

e.g. output from about 15mins at 10kHz sample rate:
Nr. samples received: 2565380
Nr. samples lost invalid pc: 4

Signed-off-by Phil Carmody <[email protected]>

---
 drivers/oprofile/cpu_buffer.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

drivers/oprofile/cpu_buffer.c +9 -3

index a7aae24..f70f954 100644
@@ -357,9 +357,15 @@ void oprofile_add_ext_sample(unsigned long pc, struct pt_regs * const regs,
 
 void oprofile_add_sample(struct pt_regs * const regs, unsigned long event)
 {
-	int is_kernel = !user_mode(regs);
-	unsigned long pc = profile_pc(regs);
-
+	int is_kernel;
+	unsigned long pc;
+	if (likely(regs)) {
+		is_kernel = !user_mode(regs);
+		pc = profile_pc(regs);
+	} else {
+		is_kernel = 0;    /* This value will not be used */
+		pc = ESCAPE_CODE; /* as this causes an early return. */
+	}
 	__oprofile_add_ext_sample(pc, regs, event, is_kernel);
 }

Signature · 1 line

1.6.0.4

Robert Richter

2010-04-28 21:15 UTC #5 lore raw

Quoted 2 lines · Phil Carmody

A few here thrashed around a couple of ideas, and the general consensus was that the following work for us, and is offered for consideration.

Phil,

I missed your patch. I will apply your version in favour of my patch since it adds statistic data.

Thanks,

-Robert

Signature · 3 lines

Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]

Robert Richter

2010-05-03 21:18 UTC #6 lore raw

In reply to Phil Carmody

Quoted 26 lines · Phil Carmody

Quoted 7 lines · ext Robert Richter
Quoted 2 lines · Siarhei Siamashka
Isn't hrtimer callback function supposed to be only called from IRQ context after this cleanup: http://lwn.net/Articles/308545/ ?
Yes, the patch is upstream since v2.6.29. Thanks Siarhei.
I will add a null pointer check anyway.
A few here thrashed around a couple of ideas, and the general consensus was that the following work for us, and is offered for consideration.
Phil
From: Phil Carmody <[email protected]>
Date: Tue, 27 Apr 2010 19:28:33 +0300
Subject: [PATCH v2 1/1] oprofile: HACK - protect from not being in an IRQ context
http://lkml.org/lkml/2010/4/27/285
Protect against dereferencing regs when it's NULL, and force a magic number into pc to prevent too deep processing. This approach permits the dropped samples to be tallied as invalid Instruction Pointer events.

Quoted 5 lines

e.g. output from about 15mins at 10kHz sample rate:
Nr. samples received: 2565380
Nr. samples lost invalid pc: 4
Signed-off-by Phil Carmody <[email protected]>

Patch applied to:

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git core

Thanks Phil!

-Robert

Signature · 3 lines

Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]

Robert Richter

2010-04-28 19:24 UTC #7 lore raw

PATCH 1/1 oprofile, hrtimer: only add samples if regs are available

In reply to Robert Richter

Quoted 7 lines · Robert Richter

Quoted 2 lines · Siarhei Siamashka
Isn't hrtimer callback function supposed to be only called from IRQ context after this cleanup: http://lwn.net/Articles/308545/ ?
Yes, the patch is upstream since v2.6.29. Thanks Siarhei.
I will add a null pointer check anyway.

Quoted 1 line

From ad8a6546b32d22ecf92b4448ad329133d18dde0f Mon Sep 17 00:00:00 2001

From: Robert Richter <[email protected]>
Date: Wed, 28 Apr 2010 20:43:18 +0200
Subject: [PATCH] oprofile, hrtimer: only add samples if regs are available

This patch adds a check if the regs pointer is valid. This actually should not happen since hrtimer notification code should always run in hard_irq context after this commit (since v2.6.29):

ca10949 hrtimer: removing all ur callback modes

However, if code does not run in interrupt context, regs will be null:

http://www.mail-archive.com/[email protected]/msg14074.html

Cc Martin Schwidefsky <[email protected]>

Signed-off-by Robert Richter <[email protected]>

---
 drivers/oprofile/timer_int.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

drivers/oprofile/timer_int.c +5 -1

index dc0ae4d..ca3436c 100644
@@ -24,8 +24,12 @@ static DEFINE_PER_CPU(struct hrtimer, oprofile_hrtimer);
 
 static enum hrtimer_restart oprofile_hrtimer_notify(struct hrtimer *hrtimer)
 {
-	oprofile_add_sample(get_irq_regs(), 0);
+	struct pt_regs *regs = get_irq_regs();
+
+	if (regs)
+		oprofile_add_sample(regs, 0);
 	hrtimer_forward_now(hrtimer, ns_to_ktime(TICK_NSEC));
+
 	return HRTIMER_RESTART;
 }

Signature · 8 lines

1.7.0.3



-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: [email protected]