[RFC][PATCH] perf_events: Fix FORK events

[RFC][PATCH] perf_events: Fix FORK events

[Peter Zijlstra]

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

Hi,

While looking into a problem reported by Pekka, I noticed that I wasn't
receiving any FORK events for a workload that did fork (see attachment).

After making the below change, stuff started working again, thing is,
I'm not sure why.

The main change is sending the FORK event to the parent instead of the
child thread, however perf_event_fork() is at the end of copy_process(),
which is after perf_event_init_task() which inherits all the counters,
so it should all have worked as it was.

We could of course just slam the commit in and not worry about it, but
that just doesn't feel right.

Alternatively, I'm really really dense and I'm totally missing the
obvious.

Signed-ff-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/perf_event.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index ab8a312..01655fa 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -3399,8 +3399,6 @@ static void perf_event_task_output(struct perf_event *event,
 	task_event->event_id.tid = perf_event_tid(event, task);
 	task_event->event_id.ptid = perf_event_tid(event, current);
 
-	task_event->event_id.time = perf_clock();
-
 	perf_output_put(&handle, task_event->event_id);
 
 	perf_output_end(&handle);
@@ -3440,7 +3438,7 @@ static void perf_event_task_event(struct perf_task_event *task_event)
 	cpuctx = &get_cpu_var(perf_cpu_context);
 	perf_event_task_ctx(&cpuctx->ctx, task_event);
 	if (!ctx)
-		ctx = rcu_dereference(task_event->task->perf_event_ctxp);
+		ctx = rcu_dereference(current->perf_event_ctxp);
 	if (ctx)
 		perf_event_task_ctx(ctx, task_event);
 	put_cpu_var(perf_cpu_context);
@@ -3471,6 +3469,7 @@ static void perf_event_task(struct task_struct *task,
 			/* .ppid */
 			/* .tid  */
 			/* .ptid */
+			.time = perf_clock(),
 		},
 	};
 


[-- Attachment #2: loop_1b_instructions-4x.c --]
[-- Type: text/x-csrc, Size: 287 bytes --]


#include <stdlib.h>
#include <stdio.h>
#include <time.h>

main ()
{
	int i;

	fork();
	fork();

	for (i = 0; i < 100000000; i++) {
		asm("nop");
		asm("nop");
		asm("nop");
		asm("nop");
		asm("nop");
		asm("nop");
		asm("nop");
	}
	wait(NULL);
	wait(NULL);
	wait(NULL);
	wait(NULL);
}

Re: [RFC][PATCH] perf_events: Fix FORK events

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> Hi,
> 
> While looking into a problem reported by Pekka, I noticed that I wasn't
> receiving any FORK events for a workload that did fork (see attachment).

ah yes, my old testcase for forks.

> After making the below change, stuff started working again, thing is, I'm 
> not sure why.

In case it matters: that workload of 4x fork (and the whole fork events 
mechanism) was always very sensitive to the precise timing of when a child 
and a parent does what, in the final dance of wait(), do_exit(), 
release_task(), etc. when a task exits. Especially on SMP systems.

> The main change is sending the FORK event to the parent instead of the 
> child thread, however perf_event_fork() is at the end of copy_process(), 
> which is after perf_event_init_task() which inherits all the counters, so 
> it should all have worked as it was.
> 
> We could of course just slam the commit in and not worry about it, but
> that just doesn't feel right.

Would be nice to figure it out ...

	Ingo

Re: [RFC][PATCH] perf_events: Fix FORK events

[Peter Zijlstra]

On Mon, 2010-02-08 at 08:44 +0100, Ingo Molnar wrote:
> 
> > We could of course just slam the commit in and not worry about it, but
> > that just doesn't feel right.
> 
> Would be nice to figure it out ... 

OK, so it is real simple, patch 22e19085 ("perf: Honour event state for
aux stream data") drops all events for !ACTIVE and newly inherited child
counters are INACTIVE.

So we can either go with the original patch and send the FORK events to
the parent because that's always ACTIVE (already true for COMM and
MMAP), or weaken the state test to < INACTIVE, which would also satisfy
Anton's case because PERF_EVENT_IOC_DISABLE sets events to OFF.

Re: [RFC][PATCH] perf_events: Fix FORK events

[Arnaldo Carvalho de Melo]

Em Mon, Feb 08, 2010 at 04:57:46PM +0100, Peter Zijlstra escreveu:
> On Mon, 2010-02-08 at 08:44 +0100, Ingo Molnar wrote:
> > 
> > > We could of course just slam the commit in and not worry about it, but
> > > that just doesn't feel right.
> > 
> > Would be nice to figure it out ... 
> 
> OK, so it is real simple, patch 22e19085 ("perf: Honour event state for
> aux stream data") drops all events for !ACTIVE and newly inherited child
> counters are INACTIVE.
> 
> So we can either go with the original patch and send the FORK events to
> the parent because that's always ACTIVE (already true for COMM and
> MMAP), or weaken the state test to < INACTIVE, which would also satisfy
> Anton's case because PERF_EVENT_IOC_DISABLE sets events to OFF.

I saw the missing forks on another testcase and after applying the
original patch I got them, I guess we should apply that one to have
2.6.33 good in this regard, if still possible to get this in.

- Arnaldo