[PATCH 0/2] perf: Fix wakeup_events logic

[PATCH 0/2] perf: Fix wakeup_events logic

[Jiri Olsa]

hi,
this patchset enables the wakeup_events check for all type
of events stored under ring buffer plus test.

Changes could be reached in here:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/aux_poll1

thanks,
jirka

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
---
Jiri Olsa (2):
      perf: Enable wakeup_events logic for all events
      perf tests: Add auxiliary events poll automated test

 kernel/events/core.c            |  14 --------
 kernel/events/ring_buffer.c     |  20 ++++++++++++
 tools/perf/Makefile             |   1 +
 tools/perf/tests/aux_poll.c     | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/builtin-test.c |   4 +++
 tools/perf/tests/tests.h        |   1 +
 6 files changed, 202 insertions(+), 14 deletions(-)
 create mode 100644 tools/perf/tests/aux_poll.c

[PATCH 1/2] perf: Enable wakeup_events logic for all events

[Jiri Olsa]

Currently the perf_events_attr::wakeup_events logic is
checked only for regular samples.

If we have an event that produce only auxiliary events
(MMAP|COMM|EXIT|FORK), the poll call does not follow
the perf_events_attr wakeup_events setup and reports
no data.

Fixing this by moving the perf_events_attr::wakeup_events
checking logic into the function and calling it from
perf_output_end. This way any output event is checked
properly.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
---
 kernel/events/core.c        | 14 --------------
 kernel/events/ring_buffer.c | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a0780b3..516eb25 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4281,20 +4281,6 @@ void perf_output_sample(struct perf_output_handle *handle,
 		}
 	}
 
-	if (!event->attr.watermark) {
-		int wakeup_events = event->attr.wakeup_events;
-
-		if (wakeup_events) {
-			struct ring_buffer *rb = handle->rb;
-			int events = local_inc_return(&rb->events);
-
-			if (events >= wakeup_events) {
-				local_sub(wakeup_events, &rb->events);
-				local_inc(&rb->wakeup);
-			}
-		}
-	}
-
 	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
 		if (data->br_stack) {
 			size_t size;
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index cd55144..1385fb1 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -54,6 +54,25 @@ static void perf_output_wakeup(struct perf_output_handle *handle)
 	irq_work_queue(&handle->event->pending);
 }
 
+static void perf_output_check_wakeup(struct perf_output_handle *handle)
+{
+	struct perf_event *event = handle->event;
+
+	if (!event->attr.watermark) {
+		int wakeup_events = event->attr.wakeup_events;
+
+		if (wakeup_events) {
+			struct ring_buffer *rb = handle->rb;
+			int events = local_inc_return(&rb->events);
+
+			if (events >= wakeup_events) {
+				local_sub(wakeup_events, &rb->events);
+				local_inc(&rb->wakeup);
+			}
+		}
+	}
+}
+
 /*
  * We need to ensure a later event_id doesn't publish a head when a former
  * event isn't done writing. However since we need to deal with NMIs we
@@ -208,6 +227,7 @@ unsigned int perf_output_skip(struct perf_output_handle *handle,
 
 void perf_output_end(struct perf_output_handle *handle)
 {
+	perf_output_check_wakeup(handle);
 	perf_output_put_handle(handle);
 	rcu_read_unlock();
 }
-- 
1.7.11.7

Re: [PATCH 1/2] perf: Enable wakeup_events logic for all events

[Peter Zijlstra]

On Thu, May 30, 2013 at 11:53:06AM +0200, Jiri Olsa wrote:
> Currently the perf_events_attr::wakeup_events logic is
> checked only for regular samples.
> 
> If we have an event that produce only auxiliary events
> (MMAP|COMM|EXIT|FORK), the poll call does not follow
> the perf_events_attr wakeup_events setup and reports
> no data.
> 
> Fixing this by moving the perf_events_attr::wakeup_events
> checking logic into the function and calling it from
> perf_output_end. This way any output event is checked
> properly.
> 

So something like this came up before and ISTR doing a patch similar to
what you propose but ended up not going with it because... uhm...

I have vague memories about people (possibly me) wanting wake_events to
mean samples. Like get me a wakeup every 20 samples. With your patch the
side-band events now also count towards 'events'.

I think the most common use of wake_events is people setting it to 1 and
have perf generate a SIGfoo for every sample so they can do magic in
their userspace signal handler. This is popular for interpreters.

It would be unfortunate to get SIGfoos for side-band chatter.


Makes sense?

Re: [PATCH 1/2] perf: Enable wakeup_events logic for all events

[Jiri Olsa]

On Thu, May 30, 2013 at 01:02:39PM +0200, Peter Zijlstra wrote:
> On Thu, May 30, 2013 at 11:53:06AM +0200, Jiri Olsa wrote:
> > Currently the perf_events_attr::wakeup_events logic is
> > checked only for regular samples.
> > 
> > If we have an event that produce only auxiliary events
> > (MMAP|COMM|EXIT|FORK), the poll call does not follow
> > the perf_events_attr wakeup_events setup and reports
> > no data.
> > 
> > Fixing this by moving the perf_events_attr::wakeup_events
> > checking logic into the function and calling it from
> > perf_output_end. This way any output event is checked
> > properly.
> > 
> 
> So something like this came up before and ISTR doing a patch similar to
> what you propose but ended up not going with it because... uhm...
> 
> I have vague memories about people (possibly me) wanting wake_events to
> mean samples. Like get me a wakeup every 20 samples. With your patch the
> side-band events now also count towards 'events'.
> 
> I think the most common use of wake_events is people setting it to 1 and
> have perf generate a SIGfoo for every sample so they can do magic in
> their userspace signal handler. This is popular for interpreters.
> 
> It would be unfortunate to get SIGfoos for side-band chatter.
> 
> 
> Makes sense?

hum, well the whole story is that for the ifunc user space tracer
(profwiz) I'd need the auxiliary stuff for the tracee (MMAP, COMM...)
so I could end up with 'reportable' perf.data file and use
just VMA addresses for functions.

So, the only way now is to create an event which is never hit
and set it up for this. At that point I noticed the poll would
never return.. hence the patch ;-)

(I was also thinking of a new special SW event doing nothing,
just be there to get aux stuff..)

Anyway, I could always check the map manually.. but I'd sort of
expect poll to work the same for anything you write/read via RB.

how about having something like:

 wakeup_events - setup for samples (stays)
 wakeup_any    - new flag saying to use wakeup_events for
                 anything going through the RB

thanks,
jirka

Re: [PATCH 1/2] perf: Enable wakeup_events logic for all events

[Arnaldo Carvalho de Melo]

Em Thu, May 30, 2013 at 01:02:39PM +0200, Peter Zijlstra escreveu:
> On Thu, May 30, 2013 at 11:53:06AM +0200, Jiri Olsa wrote:
> > Currently the perf_events_attr::wakeup_events logic is
> > checked only for regular samples.

> So something like this came up before and ISTR doing a patch similar to
> what you propose but ended up not going with it because... uhm...

> I have vague memories about people (possibly me) wanting wake_events to
> mean samples. Like get me a wakeup every 20 samples. With your patch the
> side-band events now also count towards 'events'.

I think it was me, and the test case is tools/perf/pythoh/twatch.py,
that just wants those side band events, the "fix" was:

commit cfeb1d90a1b1db96383b48888cb7a5f10ca12e12
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon Jan 30 14:43:21 2012 -0200

    perf python: Use attr.watermark in twatch.py
    
    We want to be woken up for every PERF_RECORD_ event,
attr.wakeup_events is only for PERF_RECORD_SAMPLE, so also use
attr.watermark = 1 to fix that.
 
> I think the most common use of wake_events is people setting it to 1 and
> have perf generate a SIGfoo for every sample so they can do magic in
> their userspace signal handler. This is popular for interpreters.
> 
> It would be unfortunate to get SIGfoos for side-band chatter.
> 
> 
> Makes sense?

Re: [PATCH 1/2] perf: Enable wakeup_events logic for all events

[Jiri Olsa]

On Thu, May 30, 2013 at 02:51:13PM +0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, May 30, 2013 at 01:02:39PM +0200, Peter Zijlstra escreveu:
> > On Thu, May 30, 2013 at 11:53:06AM +0200, Jiri Olsa wrote:
> > > Currently the perf_events_attr::wakeup_events logic is
> > > checked only for regular samples.
> 
> > So something like this came up before and ISTR doing a patch similar to
> > what you propose but ended up not going with it because... uhm...
> 
> > I have vague memories about people (possibly me) wanting wake_events to
> > mean samples. Like get me a wakeup every 20 samples. With your patch the
> > side-band events now also count towards 'events'.
> 
> I think it was me, and the test case is tools/perf/pythoh/twatch.py,
> that just wants those side band events, the "fix" was:
> 
> commit cfeb1d90a1b1db96383b48888cb7a5f10ca12e12
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Mon Jan 30 14:43:21 2012 -0200
> 
>     perf python: Use attr.watermark in twatch.py
>     
>     We want to be woken up for every PERF_RECORD_ event,
> attr.wakeup_events is only for PERF_RECORD_SAMPLE, so also use
> attr.watermark = 1 to fix that.

ah right, using wattermark=1.. ok ;-)

thanks,
jirka

[PATCH 2/2] perf tests: Add auxiliary events poll automated test

[Jiri Olsa]

This test creates syscall entry event restricted for
user space (we should get no samples) and accepting
auxiliary events (MMAP, COMM, EXIT, FORK).

We test the poll works properly when only auxiliary
events are received.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/Makefile             |   1 +
 tools/perf/tests/aux_poll.c     | 176 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/builtin-test.c |   4 +
 tools/perf/tests/tests.h        |   1 +
 4 files changed, 182 insertions(+)
 create mode 100644 tools/perf/tests/aux_poll.c

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 74fdd2b..64e3ce8 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -386,6 +386,7 @@ LIB_OBJS += $(OUTPUT)tests/bp_signal.o
 LIB_OBJS += $(OUTPUT)tests/bp_signal_overflow.o
 LIB_OBJS += $(OUTPUT)tests/task-exit.o
 LIB_OBJS += $(OUTPUT)tests/sw-clock.o
+LIB_OBJS += $(OUTPUT)tests/aux_poll.o
 
 BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o
 BUILTIN_OBJS += $(OUTPUT)builtin-bench.o
diff --git a/tools/perf/tests/aux_poll.c b/tools/perf/tests/aux_poll.c
new file mode 100644
index 0000000..8c5922d
--- /dev/null
+++ b/tools/perf/tests/aux_poll.c
@@ -0,0 +1,176 @@
+#include <linux/compiler.h>
+#include <signal.h>
+
+#include "evlist.h"
+#include "evsel.h"
+#include "thread_map.h"
+#include "cpumap.h"
+#include "tests.h"
+
+static int exited;
+
+static void sig_handler(int sig __maybe_unused)
+{
+	exited = 1;
+}
+
+static struct perf_evlist *aux_evlist(void)
+{
+	struct perf_evsel *evsel;
+	struct perf_evlist *evlist;
+
+	evlist = perf_evlist__new();
+	if (!evlist)
+		return NULL;
+
+	evsel = perf_evsel__newtp("raw_syscalls", "sys_enter", 0);
+	if (evsel == NULL)
+		goto error;
+
+	perf_evlist__add(evlist, evsel);
+	return evlist;
+
+ error:
+	perf_evlist__delete(evlist);
+	return NULL;
+}
+
+/*
+ * This test creates syscall entry event restricted for
+ * user space (we should get no samples) and accepting
+ * auxiliary events (MMAP, COMM, EXIT, FORK).
+ *
+ * We test the poll works properly when only auxiliary
+ * events are received.
+ *
+ */
+int test__aux_poll(void)
+{
+	int err = -1;
+	union perf_event *event;
+	struct perf_evsel *evsel;
+	struct perf_evlist *evlist;
+	struct perf_target target = {
+		.uid		= UINT_MAX,
+		.uses_mmap	= true,
+	};
+	const char *argv[] = { "true", NULL };
+	int try = 11, nr_aux = 0;
+
+	signal(SIGCHLD, sig_handler);
+	signal(SIGUSR1, sig_handler);
+
+	evlist = aux_evlist();
+	if (!evlist) {
+		pr_debug("Not enough memory|privilege to create evlist\n");
+		return -ENOMEM;
+	}
+
+	/*
+	 * Create maps of threads and cpus to monitor. In this case
+	 * we start with all threads and cpus (-1, -1) but then in
+	 * perf_evlist__prepare_workload we'll fill in the only thread
+	 * we're monitoring, the one forked there.
+	 */
+	evlist->cpus = cpu_map__dummy_new();
+	evlist->threads = thread_map__new_by_tid(-1);
+	if (!evlist->cpus || !evlist->threads) {
+		err = -ENOMEM;
+		pr_debug("Not enough memory to create thread/cpu maps\n");
+		goto out_delete_maps;
+	}
+
+	err = perf_evlist__prepare_workload(evlist, &target, argv, false, true);
+	if (err < 0) {
+		pr_debug("Couldn't run the workload!\n");
+		goto out_delete_maps;
+	}
+
+	evsel = perf_evlist__first(evlist);
+	evsel->attr.task = 1;
+	evsel->attr.mmap = 1;
+	evsel->attr.comm = 1;
+	evsel->attr.wakeup_events = 1;
+	evsel->attr.exclude_kernel = 1;
+
+	err = perf_evlist__open(evlist);
+	if (err < 0) {
+		pr_debug("Couldn't open the evlist: %s\n", strerror(-err));
+		goto out_delete_maps;
+	}
+
+	if (perf_evlist__mmap(evlist, 128, true) < 0) {
+		pr_debug("failed to mmap events: %d (%s)\n", errno,
+			 strerror(errno));
+		goto out_close_evlist;
+	}
+
+	perf_evlist__start_workload(evlist);
+
+	/*
+	 * First wait (1 second) if we get any response
+	 * via poll, if not we failed.
+	 */
+	while (--try) {
+		err = poll(evlist->pollfd, evlist->nr_fds, 100);
+		if (err > 0)
+			break;
+		else if (err < 0) {
+			if (errno == EINTR)
+				continue;
+			pr_debug("failed: poll returned %d, errno %d '%s'\n",
+				err, errno, strerror(errno));
+			goto out_close_all;
+		}
+	}
+
+	if (!try) {
+		err = -1;
+		pr_debug("failed: no poll response in 1 sec\n");
+		goto out_close_all;
+	}
+
+	pr_debug("got poll response, try %d\n", try);
+	err = 0;
+
+	/*
+	 * Now make sure that all we receive are
+	 * only auxiliary events.
+	 */
+ retry:
+	while ((event = perf_evlist__mmap_read(evlist, 0)) != NULL) {
+		switch (event->header.type) {
+		case PERF_RECORD_MMAP:
+		case PERF_RECORD_COMM:
+		case PERF_RECORD_EXIT:
+		case PERF_RECORD_FORK:
+			nr_aux++;
+			continue;
+		default:
+			pr_debug("failed: we should get only aux events\n");
+			goto out;
+		}
+	}
+
+	if (!exited || !nr_aux) {
+		poll(evlist->pollfd, evlist->nr_fds, -1);
+		goto retry;
+	}
+
+	pr_debug("received %d AUX events\n", nr_aux);
+
+ out:
+	if (nr_aux == 0) {
+		pr_debug("failed: no AUX events received\n");
+		err = -1;
+	}
+
+ out_close_all:
+	perf_evlist__munmap(evlist);
+ out_close_evlist:
+	perf_evlist__close(evlist);
+ out_delete_maps:
+	perf_evlist__delete_maps(evlist);
+	perf_evlist__delete(evlist);
+	return err;
+}
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 35b45f1466..2479be8 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -94,6 +94,10 @@ static struct test {
 		.func = test__sw_clock_freq,
 	},
 	{
+		.desc = "Test poll syscall for auxiliary events",
+		.func = test__aux_poll,
+	},
+	{
 		.func = NULL,
 	},
 };
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index dd7feae..c4dff04 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -27,5 +27,6 @@ int test__bp_signal(void);
 int test__bp_signal_overflow(void);
 int test__task_exit(void);
 int test__sw_clock_freq(void);
+int test__aux_poll(void);
 
 #endif /* TESTS_H */
-- 
1.7.11.7