Re: [RFC][PATCH 00/19] perf pmu interface changes -v4

[Peter Zijlstra <peterz@infradead.org>]

On Fri, 2010-09-10 at 14:39 +1000, Paul Mackerras wrote:
> On Tue, Sep 07, 2010 at 06:46:33PM +0200, Peter Zijlstra wrote:
> 
> > These patches prepare the perf code for multiple PMUs.
> > 
> > About the first half of these patches deals with removing all the weak
> > functions, the second half provides per pmu contexts.
> > 
> > New to the first half is Alpha, which got merged into mainline recently.
> > 
> > The second half is completely new and lightly tested, one known breakage is
> > that it currently doesn't allow to mix software and hardware events into a
> > single group (should be fixed soon).
> 
> I see Ingo committed this series into the tip perf/core branch.  Did
> this problem get fixed before that, or does it still need to be fixed?

I was thinking something like the below, except I obviously need to sort
that XXX thing.


---
 kernel/perf_event.c |   48 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 8 deletions(-)

Index: linux-2.6/kernel/perf_event.c
===================================================================
--- linux-2.6.orig/kernel/perf_event.c
+++ linux-2.6/kernel/perf_event.c
@@ -494,7 +494,7 @@ static void __perf_event_remove_from_con
 
 	raw_spin_lock(&ctx->lock);
 
-	event_sched_out(event, cpuctx, ctx);
+	group_sched_out(event, cpuctx, ctx);
 
 	list_del_event(event, ctx);
 
@@ -5522,9 +5522,10 @@ SYSCALL_DEFINE5(perf_event_open,
 	struct perf_event_context *ctx;
 	struct file *event_file = NULL;
 	struct file *group_file = NULL;
-	struct task_struct *task;
+	struct task_struct *task = NULL;
 	struct pmu *pmu;
 	int event_fd;
+	int move_group_leader = 0;
 	int fput_needed = 0;
 	int err;
 
@@ -5574,15 +5575,34 @@ SYSCALL_DEFINE5(perf_event_open,
 	 * any hardware group.
 	 */
 	pmu = event->pmu;
-	if ((pmu->task_ctx_nr == perf_sw_context) && group_leader)
-		pmu = group_leader->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 it, 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)) {
+			/*
+			 * 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_leader = 1;
+		}
+	}
 
 	/*
 	 * Get the target context (task or percpu):
 	 */
-	if (pid == -1 && cpu != -1)
-		task = NULL;
-	else
+	if (!(pid == -1 && cpu != -1))
 		task = find_lively_task_by_vpid(pid);
 
 	ctx = find_get_context(pmu, task, cpu);
@@ -5606,8 +5626,10 @@ SYSCALL_DEFINE5(perf_event_open,
 		/*
 		 * Do not allow to attach to a group in a different
 		 * task or CPU context:
+		 *
+		 * XXX fix the task vs cpu context thing for move_group_leader
 		 */
-		if (group_leader->ctx != ctx)
+		if (!move_group_leader && group_leader->ctx != ctx)
 			goto err_context;
 		/*
 		 * Only a group leader can be exclusive or pinned
@@ -5628,9 +5650,19 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_context;
 	}
 
+	if (move_group_leader) {
+		struct perf_event_context *gctx = group_leader->ctx;
+
+		mutex_lock(&gctx->mutex);
+		perf_event_remove_from_context(group_leader);
+		mutex_unlock(&gctx->mutex);
+	}
+
 	event->filp = event_file;
 	WARN_ON_ONCE(ctx->parent_ctx);
 	mutex_lock(&ctx->mutex);
+	if (move_group_leader)
+		perf_install_in_context(ctx, group_leader, cpu);
 	perf_install_in_context(ctx, event, cpu);
 	++ctx->generation;
 	mutex_unlock(&ctx->mutex);