[PATCH 1/3] perf: Make perf_event_init_context function static

[PATCH 1/3] perf: Make perf_event_init_context function static

[Jiri Olsa]

From: Jiri Olsa <jolsa@redhat.com>

Leftover from '8dc85d5 perf: Multiple task contexts'.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index cd28335..d968008 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7775,7 +7775,7 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
 /*
  * Initialize the perf_event context in task_struct
  */
-int perf_event_init_context(struct task_struct *child, int ctxn)
+static int perf_event_init_context(struct task_struct *child, int ctxn)
 {
 	struct perf_event_context *child_ctx, *parent_ctx;
 	struct perf_event_context *cloned_ctx;
-- 
1.8.3.1

[PATCH 2/3] perf: Do not allow optimized switch for non-cloned events

[Jiri Olsa]

From: Jiri Olsa <jolsa@redhat.com>

The context check in perf_event_context_sched_out allows
non-cloned context to be part of the optimized schedule
out switch.

This could move non-cloned context into another workload
child. Once this child exits, the context is closed and
leaves all original (parent) events in closed state.

Any other new cloned event will have closed state and not
measure anything. And probably causing other odd bugs.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index d968008..4e3618e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2319,7 +2319,7 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
 	next_parent = rcu_dereference(next_ctx->parent_ctx);
 
 	/* If neither context have a parent context; they cannot be clones. */
-	if (!parent && !next_parent)
+	if (!parent || !next_parent)
 		goto unlock;
 
 	if (next_parent == ctx || next_ctx == parent || next_parent == parent) {
-- 
1.8.3.1

[PATCH 3/3] perf tests: Add test to hit wrong event sched out

[Jiri Olsa]

We create single breakpoint event with inherit = true and
create many workload children.

With some luck (which is directly proportional to number of
children) we hit event optimized sched out moving original
(parent) event into one of the child, closing it and causing
wrong measured numbers.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/Makefile.perf               |  1 +
 tools/perf/tests/builtin-test.c        |  4 ++
 tools/perf/tests/optimized-sched-out.c | 90 ++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h               |  1 +
 4 files changed, 96 insertions(+)
 create mode 100644 tools/perf/tests/optimized-sched-out.c

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 9670a16..29ff74e 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -419,6 +419,7 @@ endif
 endif
 LIB_OBJS += $(OUTPUT)tests/mmap-thread-lookup.o
 LIB_OBJS += $(OUTPUT)tests/thread-mg-share.o
+LIB_OBJS += $(OUTPUT)tests/optimized-sched-out.o
 
 BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o
 BUILTIN_OBJS += $(OUTPUT)builtin-bench.o
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 6f8b01b..754dc2a 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -154,6 +154,10 @@ static struct test {
 		.func = test__hists_cumulate,
 	},
 	{
+		.desc = "Test optimized event schedule out",
+		.func = test__optimized_sched_out,
+	},
+	{
 		.func = NULL,
 	},
 };
diff --git a/tools/perf/tests/optimized-sched-out.c b/tools/perf/tests/optimized-sched-out.c
new file mode 100644
index 0000000..dfbb1b9
--- /dev/null
+++ b/tools/perf/tests/optimized-sched-out.c
@@ -0,0 +1,90 @@
+#include <linux/perf_event.h>
+#include <linux/hw_breakpoint.h>
+#include <string.h>
+#include <unistd.h>
+#include "tests.h"
+#include "perf.h"
+#include "debug.h"
+
+static void __attribute__ ((noinline)) bp_func(void)
+{
+	asm("" : : : "memory");
+}
+
+static int bp_event(void *fn)
+{
+	struct perf_event_attr pe;
+	int fd;
+
+	memset(&pe, 0, sizeof(struct perf_event_attr));
+	pe.type = PERF_TYPE_BREAKPOINT;
+	pe.size = sizeof(struct perf_event_attr);
+
+	pe.config = 0;
+	pe.bp_type = HW_BREAKPOINT_X;
+	pe.bp_addr = (unsigned long) fn;
+	pe.bp_len = sizeof(long);
+
+	pe.read_format = PERF_FORMAT_ID;
+	pe.inherit = 1;
+	pe.exclude_kernel = 1;
+	pe.exclude_hv = 1;
+	pe.disabled = 1;
+
+	fd = sys_perf_event_open(&pe, 0, -1, -1, 0);
+	if (fd < 0) {
+		pr_debug("failed opening event %llx\n", pe.config);
+		return TEST_FAIL;
+	}
+
+	return fd;
+}
+
+#define CHILDREN 100
+
+int test__optimized_sched_out(void)
+{
+	struct read_data_t {
+		__u64 val;
+		__u64 id;
+	} read_data = { 0 };
+	int fd, err, i;
+	__u64 id;
+
+	fd = bp_event(bp_func);
+	TEST_ASSERT_VAL("create event", fd >= 0);
+
+	err = ioctl(fd, PERF_EVENT_IOC_ID, &id);
+	TEST_ASSERT_VAL("id event", !err);
+
+	err = ioctl(fd, PERF_EVENT_IOC_ENABLE, 0);
+	TEST_ASSERT_VAL("enable event", !err);
+
+	for (i = 0; i < CHILDREN; i++) {
+		err = fork();
+		TEST_ASSERT_VAL("failed to fork", err != -1);
+		if (!err) {
+			pr_debug("child %d created\n", getpid());
+			bp_func();
+			exit(0);
+		}
+	}
+
+	while (wait(&err) > 0)
+		;
+
+	bp_func();
+
+	/* read */
+	err = read(fd, &read_data, sizeof(read_data));
+	TEST_ASSERT_VAL("read values", sizeof(read_data) == err);
+	TEST_ASSERT_VAL("event id", read_data.id  == id);
+
+	pr_debug("read_data.val %llu, expected %u\n",
+		  read_data.val, CHILDREN + 1);
+
+	TEST_ASSERT_VAL("event count", read_data.val == CHILDREN + 1);
+
+	close(fd);
+	return 0;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index ed64790..a62d58d 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -48,6 +48,7 @@ int test__mmap_thread_lookup(void);
 int test__thread_mg_share(void);
 int test__hists_output(void);
 int test__hists_cumulate(void);
+int test__optimized_sched_out(void);
 
 #if defined(__x86_64__) || defined(__i386__) || defined(__arm__)
 #ifdef HAVE_DWARF_UNWIND_SUPPORT
-- 
1.8.3.1

Re: [PATCH 2/3] perf: Do not allow optimized switch for non-cloned events

[Peter Zijlstra]

On Tue, Jun 24, 2014 at 10:20:25AM +0200, Jiri Olsa wrote:
> From: Jiri Olsa <jolsa@redhat.com>
> 
> The context check in perf_event_context_sched_out allows
> non-cloned context to be part of the optimized schedule
> out switch.
> 
> This could move non-cloned context into another workload
> child. Once this child exits, the context is closed and
> leaves all original (parent) events in closed state.
> 
> Any other new cloned event will have closed state and not
> measure anything. And probably causing other odd bugs.
> 
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/events/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index d968008..4e3618e 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2319,7 +2319,7 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
>  	next_parent = rcu_dereference(next_ctx->parent_ctx);
>  
>  	/* If neither context have a parent context; they cannot be clones. */
> -	if (!parent && !next_parent)
> +	if (!parent || !next_parent)
>  		goto unlock;
>  
>  	if (next_parent == ctx || next_ctx == parent || next_parent == parent) {

Ohh, nice, good catch!

Thanks

Re: [PATCH 2/3] perf: Do not allow optimized switch for non-cloned events

[Jiri Olsa]

On Tue, Jun 24, 2014 at 12:39:36PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 24, 2014 at 10:20:25AM +0200, Jiri Olsa wrote:
> > From: Jiri Olsa <jolsa@redhat.com>
> > 
> > The context check in perf_event_context_sched_out allows
> > non-cloned context to be part of the optimized schedule
> > out switch.
> > 
> > This could move non-cloned context into another workload
> > child. Once this child exits, the context is closed and
> > leaves all original (parent) events in closed state.
> > 
> > Any other new cloned event will have closed state and not
> > measure anything. And probably causing other odd bugs.
> > 
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> > Cc: David Ahern <dsahern@gmail.com>
> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/events/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index d968008..4e3618e 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -2319,7 +2319,7 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
> >  	next_parent = rcu_dereference(next_ctx->parent_ctx);
> >  
> >  	/* If neither context have a parent context; they cannot be clones. */
> > -	if (!parent && !next_parent)
> > +	if (!parent || !next_parent)
> >  		goto unlock;
> >  
> >  	if (next_parent == ctx || next_ctx == parent || next_parent == parent) {
> 
> Ohh, nice, good catch!

should I queue it with your ack?

jirka

Re: [PATCH 2/3] perf: Do not allow optimized switch for non-cloned events

[Peter Zijlstra]

On Tue, Jun 24, 2014 at 12:53:01PM +0200, Jiri Olsa wrote:
> 
> should I queue it with your ack?

I picked up the lot.

[tip:perf/urgent] perf: Do not allow optimized switch for non-cloned events

[tip-bot for Jiri Olsa]

Commit-ID:  1f9a7268c67f0290837aada443d28fd953ddca90
Gitweb:     http://git.kernel.org/tip/1f9a7268c67f0290837aada443d28fd953ddca90
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 24 Jun 2014 10:20:25 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 2 Jul 2014 08:35:56 +0200

perf: Do not allow optimized switch for non-cloned events

The context check in perf_event_context_sched_out allows
non-cloned context to be part of the optimized schedule
out switch.

This could move non-cloned context into another workload
child. Once this child exits, the context is closed and
leaves all original (parent) events in closed state.

Any other new cloned event will have closed state and not
measure anything. And probably causing other odd bugs.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: <stable@vger.kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1403598026-2310-2-git-send-email-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a33d9a2b..b0c95f0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2320,7 +2320,7 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
 	next_parent = rcu_dereference(next_ctx->parent_ctx);
 
 	/* If neither context have a parent context; they cannot be clones. */
-	if (!parent && !next_parent)
+	if (!parent || !next_parent)
 		goto unlock;
 
 	if (next_parent == ctx || next_ctx == parent || next_parent == parent) {

[tip:perf/core] perf: Make perf_event_init_context() function static

[tip-bot for Jiri Olsa]

Commit-ID:  985c8dcbe15ddd8a1b98dc6b9c6403cb5d7012ab
Gitweb:     http://git.kernel.org/tip/985c8dcbe15ddd8a1b98dc6b9c6403cb5d7012ab
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 24 Jun 2014 10:20:24 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 5 Jul 2014 11:21:52 +0200

perf: Make perf_event_init_context() function static

Leftover from '8dc85d5 perf: Multiple task contexts'.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/1403598026-2310-1-git-send-email-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a33d9a2b..67e3b9c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7776,7 +7776,7 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
 /*
  * Initialize the perf_event context in task_struct
  */
-int perf_event_init_context(struct task_struct *child, int ctxn)
+static int perf_event_init_context(struct task_struct *child, int ctxn)
 {
 	struct perf_event_context *child_ctx, *parent_ctx;
 	struct perf_event_context *cloned_ctx;