Re: [PATCH] perf cgroups: Don't rotate events for cgroups unnecessarily

[Peter Zijlstra]

On Sat, Jun 01, 2019 at 01:27:22AM -0700, Ian Rogers wrote:
> @@ -3325,6 +3331,15 @@ static int flexible_sched_in(struct perf_event *event, void *data)
>  			sid->can_add_hw = 0;
>  	}
>  
> +	/*
> +	 * If the group wasn't scheduled then set that multiplexing is necessary
> +	 * for the context. Note, this won't be set if the event wasn't
> +	 * scheduled due to event_filter_match failing due to the earlier
> +	 * return.
> +	 */
> +	if (event->state == PERF_EVENT_STATE_INACTIVE)
> +		sid->ctx->rotate_necessary = 1;
> +
>  	return 0;
>  }

That looked odd; which had me look harder at this function which
resulted in the below. Should we not terminate the context interation
the moment one flexible thingy fails to schedule?

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2314,12 +2314,8 @@ group_sched_in(struct perf_event *group_
 		return 0;
 
 	pmu->start_txn(pmu, PERF_PMU_TXN_ADD);
-
-	if (event_sched_in(group_event, cpuctx, ctx)) {
-		pmu->cancel_txn(pmu);
-		perf_mux_hrtimer_restart(cpuctx);
-		return -EAGAIN;
-	}
+	if (event_sched_in(group_event, cpuctx, ctx))
+		goto cancel;
 
 	/*
 	 * Schedule in siblings as one group (if any):
@@ -2348,10 +2344,9 @@ group_sched_in(struct perf_event *group_
 	}
 	event_sched_out(group_event, cpuctx, ctx);
 
+cancel:
 	pmu->cancel_txn(pmu);
-
 	perf_mux_hrtimer_restart(cpuctx);
-
 	return -EAGAIN;
 }
 
@@ -3317,6 +3312,7 @@ static int pinned_sched_in(struct perf_e
 static int flexible_sched_in(struct perf_event *event, void *data)
 {
 	struct sched_in_data *sid = data;
+	int ret;
 
 	if (event->state <= PERF_EVENT_STATE_OFF)
 		return 0;
@@ -3325,21 +3321,15 @@ static int flexible_sched_in(struct perf
 		return 0;
 
 	if (group_can_go_on(event, sid->cpuctx, sid->can_add_hw)) {
-		if (!group_sched_in(event, sid->cpuctx, sid->ctx))
-			list_add_tail(&event->active_list, &sid->ctx->flexible_active);
-		else
+		ret = group_sched_in(event, sid->cpuctx, sid->ctx);
+		if (ret) {
 			sid->can_add_hw = 0;
+			sid->ctx->rotate_necessary = 1;
+			return ret;
+		}
+		list_add_tail(&event->active_list, &sid->ctx->flexible_active);
 	}
 
-	/*
-	 * If the group wasn't scheduled then set that multiplexing is necessary
-	 * for the context. Note, this won't be set if the event wasn't
-	 * scheduled due to event_filter_match failing due to the earlier
-	 * return.
-	 */
-	if (event->state == PERF_EVENT_STATE_INACTIVE)
-		sid->ctx->rotate_necessary = 1;
-
 	return 0;
 }