public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Namhyung Kim <namhyung@kernel.org>
To: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Stephane Eranian <eranian@google.com>,
	Namhyung Kim <namhyung.kim@lge.com>, Jiri Olsa <jolsa@redhat.com>,
	Vince Weaver <vince@deater.net>,
	Frederic Weisbecker <fweisbec@gmail.com>
Subject: [PATCH 2/2] perf: Fix mixed hw/sw event group initialization
Date: Thu,  7 Mar 2013 13:19:50 +0900	[thread overview]
Message-ID: <1362629990-10053-2-git-send-email-namhyung@kernel.org> (raw)
In-Reply-To: <1362629990-10053-1-git-send-email-namhyung@kernel.org>

From: Namhyung Kim <namhyung.kim@lge.com>

There's a problem with mixed hw/sw group when the leader is a software
event.  For instance:

  $ perf stat -e '{task-clock,cycles,faults}' sleep 1

   Performance counter stats for 'sleep 1':

            0.273436 task-clock                #    0.000 CPUs utilized
             962,965 cycles                    #    3.522 GHz
     <not supported> faults

         1.000804279 seconds time elapsed

Jiri's patch 0231bb533675 ("perf: Fix event group context move") fixed
a part of problem but there's a devil still..

The problem arose when a sw event is added to already moved (to hw
context) group whose leader also is a sw event.  In the above example

 1. task-clock (sw event) is a group leader (has PERF_GROUP_SOFTWARE)
 2. cycles (hw event) is added, so the leader moved to the hw context
 3. faults (sw event) is added but the leader also is a sw event
 4. after find_get_context(), ctx is not same as leader->ctx since the
    leader had moved to the hw context (-EINVAL)

Fix it by adding new PERF_GROUP_MIXED flag and use leader's ctx->pmu
if it's set.

  $ perf -state -e '{task-clock,cycles,faults}' sleep 1

   Performance counter stats for 'sleep 1':

            0.670405 task-clock                #    0.001 CPUs utilized
             933,264 cycles                    #    1.392 GHz
                 176 faults                    #    0.263 M/sec

         1.001506178 seconds time elapsed

Reported-by: Andreas Hollmann <hollmann@in.tum.de>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Vince Weaver <vince@deater.net>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 include/linux/perf_event.h |  1 +
 kernel/events/core.c       | 37 ++++++++++++++++++++++---------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index e47ee462c2f2..001a3b64fe61 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -285,6 +285,7 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
 
 enum perf_group_flag {
 	PERF_GROUP_SOFTWARE		= 0x1,
+	PERF_GROUP_MIXED		= 0x2,
 };
 
 #define SWEVENT_HLIST_BITS		8
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 007dfe846d4d..06266d5ed500 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6441,6 +6441,8 @@ out:
  * @pid:		target pid
  * @cpu:		target cpu
  * @group_fd:		group leader event fd
+ * @flags:		flags which controls the meaning of arguments.
+ * 			see PERF_FLAG_*
  */
 SYSCALL_DEFINE5(perf_event_open,
 		struct perf_event_attr __user *, attr_uptr,
@@ -6536,26 +6538,30 @@ SYSCALL_DEFINE5(perf_event_open,
 	 */
 	pmu = event->pmu;
 
-	if (group_leader &&
-	    (is_software_event(event) != is_software_event(group_leader))) {
-		if (is_software_event(event)) {
-			/*
-			 * If event and group_leader are not both a software
-			 * event, and event is, then group leader is not.
-			 *
-			 * Allow the addition of software events to !software
-			 * groups, this is safe because software events never
-			 * fail to schedule.
-			 */
-			pmu = group_leader->pmu;
-		} else if (is_software_event(group_leader) &&
-			   (group_leader->group_flags & PERF_GROUP_SOFTWARE)) {
+	if (group_leader) {
+		if (group_leader->group_flags & PERF_GROUP_SOFTWARE) {
 			/*
 			 * In case the group is a pure software group, and we
 			 * try to add a hardware event, move the whole group to
 			 * the hardware context.
 			 */
-			move_group = 1;
+			if (!is_software_event(event))
+				move_group = 1;
+		} else if (group_leader->group_flags & PERF_GROUP_MIXED) {
+			/*
+			 * The group leader was moved on to a hardware context,
+			 * so move this event also.
+			 */
+			if (is_software_event(event))
+				pmu = group_leader->ctx->pmu;
+		} else if (!is_software_event(group_leader)) {
+			/*
+			 * Allow the addition of software events to !software
+			 * groups, this is safe because software events never
+			 * fail to schedule.
+			 */
+			if (is_software_event(event))
+				pmu = group_leader->pmu;
 		}
 	}
 
@@ -6650,6 +6656,7 @@ SYSCALL_DEFINE5(perf_event_open,
 			perf_install_in_context(ctx, sibling, event->cpu);
 			get_ctx(ctx);
 		}
+		group_leader->group_flags = PERF_GROUP_MIXED;
 	}
 
 	perf_install_in_context(ctx, event, event->cpu);
-- 
1.7.11.7


  reply	other threads:[~2013-03-07  4:19 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-03-07  4:19 [PATCH 1/2] perf: Reset detached siblings' group_flags Namhyung Kim
2013-03-07  4:19 ` Namhyung Kim [this message]
2013-03-11 10:01   ` [PATCH 2/2] perf: Fix mixed hw/sw event group initialization Peter Zijlstra
2013-03-11 11:20     ` Namhyung Kim
2013-03-11 13:10   ` [PATCH] perf tests: Add automated test for mixed type event groups Jiri Olsa
2013-03-11  9:59 ` [PATCH 1/2] perf: Reset detached siblings' group_flags Peter Zijlstra
2013-03-11 11:34   ` Namhyung Kim
2013-03-11 11:51   ` Namhyung Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1362629990-10053-2-git-send-email-namhyung@kernel.org \
    --to=namhyung@kernel.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=acme@ghostprotocols.net \
    --cc=eranian@google.com \
    --cc=fweisbec@gmail.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=namhyung.kim@lge.com \
    --cc=paulus@samba.org \
    --cc=vince@deater.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox