linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] perf: Do not POLLHUP event if it has children
@ 2014-09-12 11:18 Jiri Olsa
  2014-09-12 11:18 ` [PATCH 2/3] perf: Fix child event initial state setup Jiri Olsa
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Jiri Olsa @ 2014-09-12 11:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Frederic Weisbecker,
	Ingo Molnar, Paul Mackerras, Peter Zijlstra, Stephane Eranian

Currently we return POLLHUP in event polling if the monitored
process is done, but we didn't consider possible children,
that might be still running and producing data.

Before returning POLLHUP making sure that
   1) the monitored task has exited and that
   2) we don't have any children to monitor

Also adding parent wakeup when the child event is gone.

Suggested-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index d8cb4d21a346..cf779eaaf624 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3586,6 +3586,19 @@ static int perf_event_read_one(struct perf_event *event,
 	return n * sizeof(u64);
 }
 
+static bool is_event_hup(struct perf_event *event)
+{
+	bool no_children;
+
+	if (event->state != PERF_EVENT_STATE_EXIT)
+		return false;
+
+	mutex_lock(&event->child_mutex);
+	no_children = list_empty(&event->child_list);
+	mutex_unlock(&event->child_mutex);
+	return no_children;
+}
+
 /*
  * Read the performance event - simple non blocking version for now
  */
@@ -3632,7 +3645,7 @@ static unsigned int perf_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &event->waitq, wait);
 
-	if (event->state == PERF_EVENT_STATE_EXIT)
+	if (is_event_hup(event))
 		return events;
 
 	/*
@@ -7560,6 +7573,12 @@ static void sync_child_event(struct perf_event *child_event,
 	mutex_unlock(&parent_event->child_mutex);
 
 	/*
+	 * Make sure user/parent get notified, that we just
+	 * lost one event.
+	 */
+	perf_event_wakeup(parent_event);
+
+	/*
 	 * Release the parent event, if this was the last
 	 * reference to it.
 	 */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/3] perf: Fix child event initial state setup
  2014-09-12 11:18 [PATCH 1/3] perf: Do not POLLHUP event if it has children Jiri Olsa
@ 2014-09-12 11:18 ` Jiri Olsa
  2014-09-24 14:58   ` [tip:perf/core] " tip-bot for Jiri Olsa
  2014-09-12 11:18 ` [PATCH 3/3] Revert "perf: Do not allow optimized switch for non-cloned events" Jiri Olsa
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2014-09-12 11:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Frederic Weisbecker,
	Ingo Molnar, Paul Mackerras, Peter Zijlstra, Stephane Eranian,
	Jiri Olsa

From: Jiri Olsa <jolsa@redhat.com>

Currently we initialize the child event based on the original
parent state. This is wrong, because the original parent event
(and its state) is not related to current fork and also could
be already gone.

We need to initialize the child state based on the immediate
parent event state.

Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index cf779eaaf624..5a09a09b2983 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7798,6 +7798,7 @@ inherit_event(struct perf_event *parent_event,
 	      struct perf_event *group_leader,
 	      struct perf_event_context *child_ctx)
 {
+	enum perf_event_active_state parent_state = parent_event->state;
 	struct perf_event *child_event;
 	unsigned long flags;
 
@@ -7831,7 +7832,7 @@ inherit_event(struct perf_event *parent_event,
 	 * not its attr.disabled bit.  We hold the parent's mutex,
 	 * so we won't race with perf_event_{en, dis}able_family.
 	 */
-	if (parent_event->state >= PERF_EVENT_STATE_INACTIVE)
+	if (parent_state >= PERF_EVENT_STATE_INACTIVE)
 		child_event->state = PERF_EVENT_STATE_INACTIVE;
 	else
 		child_event->state = PERF_EVENT_STATE_OFF;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/3] Revert "perf: Do not allow optimized switch for non-cloned events"
  2014-09-12 11:18 [PATCH 1/3] perf: Do not POLLHUP event if it has children Jiri Olsa
  2014-09-12 11:18 ` [PATCH 2/3] perf: Fix child event initial state setup Jiri Olsa
@ 2014-09-12 11:18 ` Jiri Olsa
  2014-09-14  9:14 ` [PATCH 1/3] perf: Do not POLLHUP event if it has children Peter Zijlstra
  2014-09-24 14:58 ` [tip:perf/core] " tip-bot for Jiri Olsa
  3 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2014-09-12 11:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Frederic Weisbecker,
	Ingo Molnar, Paul Mackerras, Peter Zijlstra, Stephane Eranian,
	Jiri Olsa

From: Jiri Olsa <jolsa@redhat.com>

This reverts commit 1f9a7268c67f0290837aada443d28fd953ddca90.

With the fix of the initial state for the cloned event we now correctly
handle the error described in:
  1f9a7268c67f perf: Do not allow optimized switch for non-cloned events
so we can revert it.

I made an automated test for this, but its not suitable for automated
perf tests framework. It needs to be customized for each machine (the
more cpu the higher numbers for GROUPS/WORKERS/BYTES) and it could take
longer time to hit the issue. Adding at least a lkml link with test patch.

Link: http://lkml.kernel.org/r/20140910143535.GD2409@krava.brq.redhat.com
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
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 5a09a09b2983..532670fbfe12 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2374,7 +2374,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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] perf: Do not POLLHUP event if it has children
  2014-09-12 11:18 [PATCH 1/3] perf: Do not POLLHUP event if it has children Jiri Olsa
  2014-09-12 11:18 ` [PATCH 2/3] perf: Fix child event initial state setup Jiri Olsa
  2014-09-12 11:18 ` [PATCH 3/3] Revert "perf: Do not allow optimized switch for non-cloned events" Jiri Olsa
@ 2014-09-14  9:14 ` Peter Zijlstra
  2014-09-24 14:58 ` [tip:perf/core] " tip-bot for Jiri Olsa
  3 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2014-09-14  9:14 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: linux-kernel, Arnaldo Carvalho de Melo, Frederic Weisbecker,
	Ingo Molnar, Paul Mackerras, Stephane Eranian

On Fri, Sep 12, 2014 at 01:18:26PM +0200, Jiri Olsa wrote:
> Currently we return POLLHUP in event polling if the monitored
> process is done, but we didn't consider possible children,
> that might be still running and producing data.
> 
> Before returning POLLHUP making sure that
>    1) the monitored task has exited and that
>    2) we don't have any children to monitor
> 
> Also adding parent wakeup when the child event is gone.
> 
> Suggested-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Stephane Eranian <eranian@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Thanks Jiri (for all three patches)!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [tip:perf/core] perf: Do not POLLHUP event if it has children
  2014-09-12 11:18 [PATCH 1/3] perf: Do not POLLHUP event if it has children Jiri Olsa
                   ` (2 preceding siblings ...)
  2014-09-14  9:14 ` [PATCH 1/3] perf: Do not POLLHUP event if it has children Peter Zijlstra
@ 2014-09-24 14:58 ` tip-bot for Jiri Olsa
  3 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Jiri Olsa @ 2014-09-24 14:58 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: eranian, paulus, acme, linux-kernel, hpa, mingo, jolsa, torvalds,
	a.p.zijlstra, peterz, fweisbec, tglx

Commit-ID:  dc633982ff3f4fd74cdc11b5a6ae53d39a0b2451
Gitweb:     http://git.kernel.org/tip/dc633982ff3f4fd74cdc11b5a6ae53d39a0b2451
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Fri, 12 Sep 2014 13:18:26 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Sep 2014 14:48:11 +0200

perf: Do not POLLHUP event if it has children

Currently we return POLLHUP in event polling if the monitored
process is done, but we didn't consider possible children,
that might be still running and producing data.

Before returning POLLHUP making sure that:

   1) the monitored task has exited and that
   2) we don't have any children to monitor

Also adding parent wakeup when the child event is gone.

Suggested-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1410520708-19275-1-git-send-email-jolsa@kernel.org
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 733c616..15e58d4 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3587,6 +3587,19 @@ static int perf_event_read_one(struct perf_event *event,
 	return n * sizeof(u64);
 }
 
+static bool is_event_hup(struct perf_event *event)
+{
+	bool no_children;
+
+	if (event->state != PERF_EVENT_STATE_EXIT)
+		return false;
+
+	mutex_lock(&event->child_mutex);
+	no_children = list_empty(&event->child_list);
+	mutex_unlock(&event->child_mutex);
+	return no_children;
+}
+
 /*
  * Read the performance event - simple non blocking version for now
  */
@@ -3632,7 +3645,7 @@ static unsigned int perf_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &event->waitq, wait);
 
-	if (event->state == PERF_EVENT_STATE_EXIT)
+	if (is_event_hup(event))
 		return events;
 
 	/*
@@ -7580,6 +7593,12 @@ static void sync_child_event(struct perf_event *child_event,
 	mutex_unlock(&parent_event->child_mutex);
 
 	/*
+	 * Make sure user/parent get notified, that we just
+	 * lost one event.
+	 */
+	perf_event_wakeup(parent_event);
+
+	/*
 	 * Release the parent event, if this was the last
 	 * reference to it.
 	 */

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [tip:perf/core] perf: Fix child event initial state setup
  2014-09-12 11:18 ` [PATCH 2/3] perf: Fix child event initial state setup Jiri Olsa
@ 2014-09-24 14:58   ` tip-bot for Jiri Olsa
  0 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Jiri Olsa @ 2014-09-24 14:58 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, eranian, hpa, mingo, jolsa, torvalds, peterz, acme,
	jolsa, fweisbec, tglx

Commit-ID:  1929def9e609d1a8cdb1626d85eda3da66921a7d
Gitweb:     http://git.kernel.org/tip/1929def9e609d1a8cdb1626d85eda3da66921a7d
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Fri, 12 Sep 2014 13:18:27 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Sep 2014 14:48:12 +0200

perf: Fix child event initial state setup

Currently we initialize the child event based on the original
parent state. This is wrong, because the original parent event
(and its state) is not related to current fork and also could
be already gone.

We need to initialize the child state based on the immediate
parent event state.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1410520708-19275-2-git-send-email-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 15e58d4..132524c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7818,6 +7818,7 @@ inherit_event(struct perf_event *parent_event,
 	      struct perf_event *group_leader,
 	      struct perf_event_context *child_ctx)
 {
+	enum perf_event_active_state parent_state = parent_event->state;
 	struct perf_event *child_event;
 	unsigned long flags;
 
@@ -7851,7 +7852,7 @@ inherit_event(struct perf_event *parent_event,
 	 * not its attr.disabled bit.  We hold the parent's mutex,
 	 * so we won't race with perf_event_{en, dis}able_family.
 	 */
-	if (parent_event->state >= PERF_EVENT_STATE_INACTIVE)
+	if (parent_state >= PERF_EVENT_STATE_INACTIVE)
 		child_event->state = PERF_EVENT_STATE_INACTIVE;
 	else
 		child_event->state = PERF_EVENT_STATE_OFF;

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-09-24 14:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-12 11:18 [PATCH 1/3] perf: Do not POLLHUP event if it has children Jiri Olsa
2014-09-12 11:18 ` [PATCH 2/3] perf: Fix child event initial state setup Jiri Olsa
2014-09-24 14:58   ` [tip:perf/core] " tip-bot for Jiri Olsa
2014-09-12 11:18 ` [PATCH 3/3] Revert "perf: Do not allow optimized switch for non-cloned events" Jiri Olsa
2014-09-14  9:14 ` [PATCH 1/3] perf: Do not POLLHUP event if it has children Peter Zijlstra
2014-09-24 14:58 ` [tip:perf/core] " tip-bot for Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).