Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 21 Jun 2019 10:24:22 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Ian Rogers <irogers@google.com>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-kernel@vger.kernel.org,
        Kan Liang <kan.liang@linux.intel.com>, ak@linux.intel.com,
        eranian@google.com
Subject: Re: [PATCH] perf cgroups: Don't rotate events for cgroups
 unnecessarily
Message-ID: <20190621082422.GH3436@hirez.programming.kicks-ass.net>
References: <20190601082722.44543-1-irogers@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190601082722.44543-1-irogers@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;
 }