[GIT PULL] perf fixes

[GIT PULL] perf fixes

[Frederic Weisbecker]

Ingo,

Please pull the perf/urgent branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	perf/urgent

Thanks,
	Frederic
---

Frederic Weisbecker (2):
      perf: Correctly align perf event tracing buffer
      perf: Use hot regs with software sched switch/migrate events


 include/linux/perf_event.h      |   21 ++++++++++++++-------
 kernel/perf_event.c             |    4 +---
 kernel/trace/trace_event_perf.c |   11 +++++++++--
 3 files changed, 24 insertions(+), 12 deletions(-)

[PATCH 1/2] perf: Correctly align perf event tracing buffer

[Frederic Weisbecker]

The trace event buffer used by perf to record raw sample events
is typed as an array of char and may then not be aligned to 8
by alloc_percpu().

But we need it to be aligned to 8 in sparc64 because we cast
this buffer into a random structure type built by the TRACE_EVENT()
macro to store the traces. So if a random 64 bits field is accessed
inside, it may be not under an expected good alignment.

Use an array of long instead to force the appropriate alignment, and
perform a compile time check to ensure the size in byte of the buffer
is a multiple of sizeof(long) so that its actual size doesn't get
shrinked under us.

This fixes unaligned accesses reported while using perf lock
in sparc 64.

Suggested-by: David Miller <davem@davemloft.net>
Suggested-by: Tejun Heo <htejun@gmail.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: David Miller <davem@davemloft.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_event_perf.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 0709e4f..69941f3 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -15,7 +15,12 @@ EXPORT_PER_CPU_SYMBOL_GPL(perf_trace_regs);
 static char *perf_trace_buf;
 static char *perf_trace_buf_nmi;
 
-typedef typeof(char [PERF_MAX_TRACE_SIZE]) perf_trace_t ;
+/*
+ * Force it to be aligned to unsigned long to avoid misaligned accesses
+ * suprises
+ */
+typedef typeof(unsigned long [PERF_MAX_TRACE_SIZE / sizeof(unsigned long)])
+	perf_trace_t;
 
 /* Count the events in use (per event id, not per instance) */
 static int	total_ref_count;
@@ -128,6 +133,8 @@ __kprobes void *perf_trace_buf_prepare(int size, unsigned short type,
 	char *trace_buf, *raw_data;
 	int pc, cpu;
 
+	BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
+
 	pc = preempt_count();
 
 	/* Protect the per cpu buffer, begin the rcu read side */
@@ -150,7 +157,7 @@ __kprobes void *perf_trace_buf_prepare(int size, unsigned short type,
 	raw_data = per_cpu_ptr(trace_buf, cpu);
 
 	/* zero the dead bytes from align to not leak stack to user */
-	*(u64 *)(&raw_data[size - sizeof(u64)]) = 0ULL;
+	memset(&raw_data[size - sizeof(u64)], 0, sizeof(u64));
 
 	entry = (struct trace_entry *)raw_data;
 	tracing_generic_entry_update(entry, *irq_flags, pc);
-- 
1.6.2.3

[PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Frederic Weisbecker]

Scheduler's task migration events don't work because they always
pass NULL regs perf_sw_event(). The event hence gets filtered
in perf_swevent_add().

Scheduler's context switches events use task_pt_regs() to get
the context when the event occured which is a wrong thing to
do as this won't give us the place in the kernel where we went
to sleep but the place where we left userspace. The result is
even more wrong if we switch from a kernel thread.

Use the hot regs snapshot for both events as they belong to the
non-interrupt/exception based events family. Unlike page faults
or so that provide the regs matching the exact origin of the event,
we need to save the current context.

This makes the task migration event working and fix the context
switch callchains and origin ip.

Example: perf record -a -e cs

Before:

    10.91%      ksoftirqd/0                  0  [k] 0000000000000000
                |
                --- (nil)
                    perf_callchain
                    perf_prepare_sample
                    __perf_event_overflow
                    perf_swevent_overflow
                    perf_swevent_add
                    perf_swevent_ctx_event
                    do_perf_sw_event
                    __perf_sw_event
                    perf_event_task_sched_out
                    schedule
                    run_ksoftirqd
                    kthread
                    kernel_thread_helper

After:

    23.77%  hald-addon-stor  [kernel.kallsyms]  [k] schedule
            |
            --- schedule
               |
               |--60.00%-- schedule_timeout
               |          wait_for_common
               |          wait_for_completion
               |          blk_execute_rq
               |          scsi_execute
               |          scsi_execute_req
               |          sr_test_unit_ready
               |          |
               |          |--66.67%-- sr_media_change
               |          |          media_changed
               |          |          cdrom_media_changed
               |          |          sr_block_media_changed
               |          |          check_disk_change
               |          |          cdrom_open

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: David Miller <davem@davemloft.net>
---
 include/linux/perf_event.h |   21 ++++++++++++++-------
 kernel/perf_event.c        |    4 +---
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 9547703..c8e3754 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -842,13 +842,6 @@ extern atomic_t perf_swevent_enabled[PERF_COUNT_SW_MAX];
 
 extern void __perf_sw_event(u32, u64, int, struct pt_regs *, u64);
 
-static inline void
-perf_sw_event(u32 event_id, u64 nr, int nmi, struct pt_regs *regs, u64 addr)
-{
-	if (atomic_read(&perf_swevent_enabled[event_id]))
-		__perf_sw_event(event_id, nr, nmi, regs, addr);
-}
-
 extern void
 perf_arch_fetch_caller_regs(struct pt_regs *regs, unsigned long ip, int skip);
 
@@ -887,6 +880,20 @@ static inline void perf_fetch_caller_regs(struct pt_regs *regs, int skip)
 	return perf_arch_fetch_caller_regs(regs, ip, skip);
 }
 
+static inline void
+perf_sw_event(u32 event_id, u64 nr, int nmi, struct pt_regs *regs, u64 addr)
+{
+	if (atomic_read(&perf_swevent_enabled[event_id])) {
+		struct pt_regs hot_regs;
+
+		if (!regs) {
+			perf_fetch_caller_regs(&hot_regs, 1);
+			regs = &hot_regs;
+		}
+		__perf_sw_event(event_id, nr, nmi, regs, addr);
+	}
+}
+
 extern void __perf_event_mmap(struct vm_area_struct *vma);
 
 static inline void perf_event_mmap(struct vm_area_struct *vma)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index fb3031c..bc7943c 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -1164,11 +1164,9 @@ void perf_event_task_sched_out(struct task_struct *task,
 	struct perf_event_context *ctx = task->perf_event_ctxp;
 	struct perf_event_context *next_ctx;
 	struct perf_event_context *parent;
-	struct pt_regs *regs;
 	int do_switch = 1;
 
-	regs = task_pt_regs(task);
-	perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, 1, regs, 0);
+	perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, 1, NULL, 0);
 
 	if (likely(!ctx || !cpuctx->task_ctx))
 		return;
-- 
1.6.2.3

Re: [GIT PULL] perf fixes

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Ingo,
> 
> Please pull the perf/urgent branch that can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	perf/urgent
> 
> Thanks,
> 	Frederic
> ---
> 
> Frederic Weisbecker (2):
>       perf: Correctly align perf event tracing buffer
>       perf: Use hot regs with software sched switch/migrate events
> 
> 
>  include/linux/perf_event.h      |   21 ++++++++++++++-------
>  kernel/perf_event.c             |    4 +---
>  kernel/trace/trace_event_perf.c |   11 +++++++++--
>  3 files changed, 24 insertions(+), 12 deletions(-)

Pulled, thanks a lot Frederic!

	Ingo

Re: [PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Peter Zijlstra]

On Sun, 2010-03-28 at 07:11 +0200, Frederic Weisbecker wrote:
> Scheduler's task migration events don't work because they always
> pass NULL regs perf_sw_event(). The event hence gets filtered
> in perf_swevent_add().
> 
> Scheduler's context switches events use task_pt_regs() to get
> the context when the event occured which is a wrong thing to
> do as this won't give us the place in the kernel where we went
> to sleep but the place where we left userspace. The result is
> even more wrong if we switch from a kernel thread.
> 
> Use the hot regs snapshot for both events as they belong to the
> non-interrupt/exception based events family. Unlike page faults
> or so that provide the regs matching the exact origin of the event,
> we need to save the current context.
> 
> This makes the task migration event working and fix the context
> switch callchains and origin ip.


But after this its no longer possible to profile userspace on context
switches is it?

Re: [PATCH 1/2] perf: Correctly align perf event tracing buffer

[Peter Zijlstra]

On Sun, 2010-03-28 at 07:11 +0200, Frederic Weisbecker wrote:
> The trace event buffer used by perf to record raw sample events
> is typed as an array of char and may then not be aligned to 8
> by alloc_percpu().
> 
> But we need it to be aligned to 8 in sparc64 because we cast
> this buffer into a random structure type built by the TRACE_EVENT()
> macro to store the traces. So if a random 64 bits field is accessed
> inside, it may be not under an expected good alignment.
> 
> Use an array of long instead to force the appropriate alignment, and
> perform a compile time check to ensure the size in byte of the buffer
> is a multiple of sizeof(long) so that its actual size doesn't get
> shrinked under us.
> 
> This fixes unaligned accesses reported while using perf lock
> in sparc 64.
> 
> Suggested-by: David Miller <davem@davemloft.net>
> Suggested-by: Tejun Heo <htejun@gmail.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: David Miller <davem@davemloft.net>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_event_perf.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index 0709e4f..69941f3 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -15,7 +15,12 @@ EXPORT_PER_CPU_SYMBOL_GPL(perf_trace_regs);
>  static char *perf_trace_buf;
>  static char *perf_trace_buf_nmi;
>  
> -typedef typeof(char [PERF_MAX_TRACE_SIZE]) perf_trace_t ;
> +/*
> + * Force it to be aligned to unsigned long to avoid misaligned accesses
> + * suprises
> + */
> +typedef typeof(unsigned long [PERF_MAX_TRACE_SIZE / sizeof(unsigned long)])
> +	perf_trace_t;
>  

wouldn't __aligned(8) be simpler?

Re: [PATCH 1/2] perf: Correctly align perf event tracing buffer

[Frederic Weisbecker]

On Mon, Mar 29, 2010 at 10:51:31AM +0200, Peter Zijlstra wrote:
> On Sun, 2010-03-28 at 07:11 +0200, Frederic Weisbecker wrote:
> >  kernel/trace/trace_event_perf.c |   11 +++++++++--
> >  1 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > index 0709e4f..69941f3 100644
> > --- a/kernel/trace/trace_event_perf.c
> > +++ b/kernel/trace/trace_event_perf.c
> > @@ -15,7 +15,12 @@ EXPORT_PER_CPU_SYMBOL_GPL(perf_trace_regs);
> >  static char *perf_trace_buf;
> >  static char *perf_trace_buf_nmi;
> >  
> > -typedef typeof(char [PERF_MAX_TRACE_SIZE]) perf_trace_t ;
> > +/*
> > + * Force it to be aligned to unsigned long to avoid misaligned accesses
> > + * suprises
> > + */
> > +typedef typeof(unsigned long [PERF_MAX_TRACE_SIZE / sizeof(unsigned long)])
> > +	perf_trace_t;
> >  
> 
> wouldn't __aligned(8) be simpler?


David and Tejun seemed to prefer to create the alignment on the
type level rather than using an align.

I'm personally fine either way.

Re: [PATCH 1/2] perf: Correctly align perf event tracing buffer

[Peter Zijlstra]

On Mon, 2010-03-29 at 19:16 +0200, Frederic Weisbecker wrote:
> On Mon, Mar 29, 2010 at 10:51:31AM +0200, Peter Zijlstra wrote:
> > On Sun, 2010-03-28 at 07:11 +0200, Frederic Weisbecker wrote:
> > >  kernel/trace/trace_event_perf.c |   11 +++++++++--
> > >  1 files changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > > index 0709e4f..69941f3 100644
> > > --- a/kernel/trace/trace_event_perf.c
> > > +++ b/kernel/trace/trace_event_perf.c
> > > @@ -15,7 +15,12 @@ EXPORT_PER_CPU_SYMBOL_GPL(perf_trace_regs);
> > >  static char *perf_trace_buf;
> > >  static char *perf_trace_buf_nmi;
> > >  
> > > -typedef typeof(char [PERF_MAX_TRACE_SIZE]) perf_trace_t ;
> > > +/*
> > > + * Force it to be aligned to unsigned long to avoid misaligned accesses
> > > + * suprises
> > > + */
> > > +typedef typeof(unsigned long [PERF_MAX_TRACE_SIZE / sizeof(unsigned long)])
> > > +	perf_trace_t;
> > >  
> > 
> > wouldn't __aligned(8) be simpler?
> 
> 
> David and Tejun seemed to prefer to create the alignment on the
> type level rather than using an align.
> 
> I'm personally fine either way.

Also, if you need u64 alignment, shouldn't you use u64 instead of
unsigned long, the alignment requirement on those two might differ on
32bit machines.

Re: [PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Frederic Weisbecker]

On Mon, Mar 29, 2010 at 10:49:59AM +0200, Peter Zijlstra wrote:
> On Sun, 2010-03-28 at 07:11 +0200, Frederic Weisbecker wrote:
> > Scheduler's task migration events don't work because they always
> > pass NULL regs perf_sw_event(). The event hence gets filtered
> > in perf_swevent_add().
> > 
> > Scheduler's context switches events use task_pt_regs() to get
> > the context when the event occured which is a wrong thing to
> > do as this won't give us the place in the kernel where we went
> > to sleep but the place where we left userspace. The result is
> > even more wrong if we switch from a kernel thread.
> > 
> > Use the hot regs snapshot for both events as they belong to the
> > non-interrupt/exception based events family. Unlike page faults
> > or so that provide the regs matching the exact origin of the event,
> > we need to save the current context.
> > 
> > This makes the task migration event working and fix the context
> > switch callchains and origin ip.
> 
> 
> But after this its no longer possible to profile userspace on context
> switches is it?


Once the callchain on the kernel finishes, we bounce to the userspace
part, using task_pt_regs(). The previous version was incorrect because
it was ignoring the kernel part.

But you makes me wonder... We don't take into account exclude_kernel
or exclude_user with these hot regs.

I think we need several new things:

Every arch does its own:

	if (!is_user)
		perf_callchain_kernel(regs, entry);

	if (current->mm)
		perf_callchain_user(regs, entry);

Plus perf_callchain_user() goes fetching task_pt_regs()
by itself.

This is a check we should do from the core, according
to exclude_kernel, exclude_user, user_mode and current->mm

Archs shouldn't bother about these details.
They should just implement perf_callchain_kernel and perf_callchain_user
rather than a monolithic one that deals with contexts.

Each time we pass regs to perf_event_overflow() we should call
a perf_filter_callchain(struct pt_regs *default) that checks the
exclude_* things and override with task_pt_regs() if needed
(and if current->mm is set) so that even the ip source will
be correct.

And a generic perf_callchain() can deal with perf_callchain_kernel()
and perf_callchain_user() calls, again, according the exclude_*
policies.

I'm going to make a quick fix for perf_fetch_caller_regs() that
passes task_pt_regs if exclude_kernel for perf/urgent,
and I'll do the above cleanups/invasive fixes on perf/core.

Re: [PATCH 1/2] perf: Correctly align perf event tracing buffer

[Frederic Weisbecker]

On Mon, Mar 29, 2010 at 07:20:09PM +0200, Peter Zijlstra wrote:
> On Mon, 2010-03-29 at 19:16 +0200, Frederic Weisbecker wrote:
> > On Mon, Mar 29, 2010 at 10:51:31AM +0200, Peter Zijlstra wrote:
> > > On Sun, 2010-03-28 at 07:11 +0200, Frederic Weisbecker wrote:
> > > >  kernel/trace/trace_event_perf.c |   11 +++++++++--
> > > >  1 files changed, 9 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > > > index 0709e4f..69941f3 100644
> > > > --- a/kernel/trace/trace_event_perf.c
> > > > +++ b/kernel/trace/trace_event_perf.c
> > > > @@ -15,7 +15,12 @@ EXPORT_PER_CPU_SYMBOL_GPL(perf_trace_regs);
> > > >  static char *perf_trace_buf;
> > > >  static char *perf_trace_buf_nmi;
> > > >  
> > > > -typedef typeof(char [PERF_MAX_TRACE_SIZE]) perf_trace_t ;
> > > > +/*
> > > > + * Force it to be aligned to unsigned long to avoid misaligned accesses
> > > > + * suprises
> > > > + */
> > > > +typedef typeof(unsigned long [PERF_MAX_TRACE_SIZE / sizeof(unsigned long)])
> > > > +	perf_trace_t;
> > > >  
> > > 
> > > wouldn't __aligned(8) be simpler?
> > 
> > 
> > David and Tejun seemed to prefer to create the alignment on the
> > type level rather than using an align.
> > 
> > I'm personally fine either way.
> 
> Also, if you need u64 alignment, shouldn't you use u64 instead of
> unsigned long, the alignment requirement on those two might differ on
> 32bit machines.


Yeah but we need the u64 alignment only on 64 bits machines.

Re: [PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Peter Zijlstra]

On Mon, 2010-03-29 at 19:47 +0200, Frederic Weisbecker wrote:
> 
> 
> I'm going to make a quick fix for perf_fetch_caller_regs() that
> passes task_pt_regs if exclude_kernel for perf/urgent,
> and I'll do the above cleanups/invasive fixes on perf/core.
> 
> 
ok, sounds sensible, thanks!

Re: [PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Frederic Weisbecker]

On Mon, Mar 29, 2010 at 08:05:38PM +0200, Peter Zijlstra wrote:
> On Mon, 2010-03-29 at 19:47 +0200, Frederic Weisbecker wrote:
> > 
> > 
> > I'm going to make a quick fix for perf_fetch_caller_regs() that
> > passes task_pt_regs if exclude_kernel for perf/urgent,
> > and I'll do the above cleanups/invasive fixes on perf/core.
> > 
> > 
> ok, sounds sensible, thanks!


Actually I have doubts about what should be the strict sense
of exclude_kernel.

Does that mean we exclude any event that happened in the kernel?
Or does that mean we exclude the part that happened in the kernel?

Depending on the case, we do either.

In perf_swevent_hrtimer(), we simply go back to task_pt_regs()
if exclude_kernel.

But in other software events, we don't such fix, we actually
filter out the event if it is not user_mode().

So, I'm a bit confused on what to do.
I'm tempted to adopt the meaning from perf_swevent_hrtimer()
for software events too, I'm not sure...

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index b0feb47..3cb5de8 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -3986,14 +3986,17 @@ static int perf_tp_event_match(struct perf_event *event,
 				struct perf_sample_data *data);
 
 static int perf_exclude_event(struct perf_event *event,
-			      struct pt_regs *regs)
+			      struct pt_regs **regs)
 {
-	if (regs) {
-		if (event->attr.exclude_user && user_mode(regs))
+	if (*regs) {
+		if (event->attr.exclude_user && user_mode(*regs))
 			return 1;
 
-		if (event->attr.exclude_kernel && !user_mode(regs))
-			return 1;
+		if (event->attr.exclude_kernel && !user_mode(*regs))
+			if (current->mm)
+				*regs = task_pt_regs();
+			else
+				return 1;
 	}
 
 	return 0;
@@ -4017,7 +4020,7 @@ static int perf_swevent_match(struct perf_event *event,
 	if (event->attr.config != event_id)
 		return 0;
 
-	if (perf_exclude_event(event, regs))
+	if (perf_exclude_event(event, &regs))
 		return 0;
 
 	if (event->attr.type == PERF_TYPE_TRACEPOINT &&
@@ -4442,7 +4445,7 @@ void perf_bp_event(struct perf_event *bp, void *data)
 
 	perf_sample_data_init(&sample, bp->attr.bp_addr);
 
-	if (!perf_exclude_event(bp, regs))
+	if (!perf_exclude_event(bp, &regs))
 		perf_swevent_add(bp, 1, 1, &sample, regs);
 }
 #else

Re: [PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Frederic Weisbecker]

On Tue, Mar 30, 2010 at 12:43:54AM +0200, Frederic Weisbecker wrote:
> On Mon, Mar 29, 2010 at 08:05:38PM +0200, Peter Zijlstra wrote:
> > On Mon, 2010-03-29 at 19:47 +0200, Frederic Weisbecker wrote:
> > > 
> > > 
> > > I'm going to make a quick fix for perf_fetch_caller_regs() that
> > > passes task_pt_regs if exclude_kernel for perf/urgent,
> > > and I'll do the above cleanups/invasive fixes on perf/core.
> > > 
> > > 
> > ok, sounds sensible, thanks!
> 
> 
> Actually I have doubts about what should be the strict sense
> of exclude_kernel.
> 
> Does that mean we exclude any event that happened in the kernel?
> Or does that mean we exclude the part that happened in the kernel?
> 
> Depending on the case, we do either.
> 
> In perf_swevent_hrtimer(), we simply go back to task_pt_regs()
> if exclude_kernel.
> 
> But in other software events, we don't such fix, we actually
> filter out the event if it is not user_mode().
> 
> So, I'm a bit confused on what to do.
> I'm tempted to adopt the meaning from perf_swevent_hrtimer()
> for software events too, I'm not sure...


I think this is the right thing to do: jump back to user context
instead of filtering out (unless kernel thread).

Otherwise every software events, trace events included, are totally
pointless with exclude_kernel.

Re: [PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Peter Zijlstra]

On Tue, 2010-03-30 at 00:43 +0200, Frederic Weisbecker wrote:

> Actually I have doubts about what should be the strict sense
> of exclude_kernel.
> 
> Does that mean we exclude any event that happened in the kernel?
> Or does that mean we exclude the part that happened in the kernel?
> 
> Depending on the case, we do either.
> 
> In perf_swevent_hrtimer(), we simply go back to task_pt_regs()
> if exclude_kernel.
> 
> But in other software events, we don't such fix, we actually
> filter out the event if it is not user_mode().
> 
> So, I'm a bit confused on what to do.
> I'm tempted to adopt the meaning from perf_swevent_hrtimer()
> for software events too, I'm not sure...

Yes, that is indeed a good point. Problem is that perf_swevent_hrtimer()
is not quite correct either, since strictly speaking its timeline should
stop on the excluded region, but implementing that would make context
switches horribly expensive.

That said, the option that would be most correct is to simply not count
these events, and in that respect the current behaviour seems best.

Maybe we can make a new perf feature that would for each kernel event
(hw pmu included) report on the userspace state, would that be useful?

Re: [PATCH 2/2] perf: Use hot regs with software sched switch/migrate events

[Frederic Weisbecker]

On Tue, Mar 30, 2010 at 08:54:52PM +0200, Peter Zijlstra wrote:
> On Tue, 2010-03-30 at 00:43 +0200, Frederic Weisbecker wrote:
> 
> > Actually I have doubts about what should be the strict sense
> > of exclude_kernel.
> > 
> > Does that mean we exclude any event that happened in the kernel?
> > Or does that mean we exclude the part that happened in the kernel?
> > 
> > Depending on the case, we do either.
> > 
> > In perf_swevent_hrtimer(), we simply go back to task_pt_regs()
> > if exclude_kernel.
> > 
> > But in other software events, we don't such fix, we actually
> > filter out the event if it is not user_mode().
> > 
> > So, I'm a bit confused on what to do.
> > I'm tempted to adopt the meaning from perf_swevent_hrtimer()
> > for software events too, I'm not sure...
> 
> Yes, that is indeed a good point. Problem is that perf_swevent_hrtimer()
> is not quite correct either, since strictly speaking its timeline should
> stop on the excluded region, but implementing that would make context
> switches horribly expensive.



No we wouldn't need that. We would just need to change the regs
check.

Currently we have this:

	regs = get_irq_regs();
	/*
	 * In case we exclude kernel IPs or are somehow not in interrupt
	 * context, provide the next best thing, the user IP.
	 */
	if ((event->attr.exclude_kernel || !regs) &&
			!event->attr.exclude_user)
		regs = task_pt_regs(current);


According to the strict meaning of exclude_kernel (event that happened in
userspace), we should have this:


	regs = get_irq_regs();

	if ((event->attr.exclude_kernel && regs)
		return ret;

	if (!regs && !event->attr.exclude_user && current->mm)
		regs = task_pt_regs(current);

	if (regs)
		overflow()


Note the current code is also buggy because we call task_pt_regs()
whenever we are a kernel thread or not.


> 
> That said, the option that would be most correct is to simply not count
> these events, and in that respect the current behaviour seems best.


Ok. But in this case I'm not sure what to do with the context switch
software event. The new hot regs thing now capture the kernel context,
whereas before it was only capturing userspace exit point.

Are you fine with that? The callchain will still go to userspace too.


> Maybe we can make a new perf feature that would for each kernel event
> (hw pmu included) report on the userspace state, would that be useful?


I'm not sure it would be useful...