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:   Wed, 20 Mar 2019 14:11:08 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Stephane Eranian <eranian@google.com>
Cc:     Ingo Molnar <mingo@kernel.org>, Jiri Olsa <jolsa@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>, tonyj@suse.com,
        nelson.dsouza@intel.com
Subject: Re: [RFC][PATCH 7/8] perf/x86: Optimize x86_schedule_events()
Message-ID: <20190320131108.GG6058@hirez.programming.kicks-ass.net>
References: <20190314130113.919278615@infradead.org>
 <20190314130706.061994422@infradead.org>
 <CABPqkBSH5UBaV7+0JKgr4YmEke8Tu4Hry9GAFYT5C_gsncqf3A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CABPqkBSH5UBaV7+0JKgr4YmEke8Tu4Hry9GAFYT5C_gsncqf3A@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Mar 19, 2019 at 04:55:16PM -0700, Stephane Eranian wrote:
> On Thu, Mar 14, 2019 at 6:11 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > @@ -858,8 +864,20 @@ int x86_schedule_events(struct cpu_hw_ev
> >                 x86_pmu.start_scheduling(cpuc);
> >
> >         for (i = 0, wmin = X86_PMC_IDX_MAX, wmax = 0; i < n; i++) {
> > -               c = x86_pmu.get_event_constraints(cpuc, i, cpuc->event_list[i]);
> > -               cpuc->event_constraint[i] = c;
> > +               c = cpuc->event_constraint[i];
> > +
> > +               /*
> > +                * Request constraints for new events; or for those events that
> > +                * have a dynamic constraint due to the HT workaround -- for
> > +                * those the constraint can change due to scheduling activity
> > +                * on the other sibling.
> > +                */
> > +               if (!c || ((c->flags & PERF_X86_EVENT_DYNAMIC) &&
> > +                          is_ht_workaround_active(cpuc))) {
> > +
> > +                       c = x86_pmu.get_event_constraints(cpuc, i, cpuc->event_list[i]);
> > +                       cpuc->event_constraint[i] = c;
> > +               }

> On this one, I think there may be a problem with events with
> shared_regs constraints.

Hmm...

> Constraint is dynamic as it depends on other events which share the
> same MSR, yet it is not marked as DYNAMIC.

it returns &emptyconstraint or a table constraint, depending on register
state.

> But this may be okay because these other events are all on the same
> CPU and thus scheduled during the same ctx_sched_in(). Yet with the
> swapping in intel_alt_er(), we need to double-check that we cannot
> reuse a constraint which could be stale.

> I believe this is okay, just double-check.

I'm not sure I see a problem.

So if we're the first event on a shared register, we claim the register
and scheduling succeeds (barring other constraints).

If we're the second event on a shared register (and have conflicting
register state), we get the empty constraint. This _will_ cause
scheduling to fail. We'll not cache the state and punt it back to the
core code.

So no future scheduling pass will come to see a shared reg constraint
that could've changed.

Now, there is indeed the intel_alt_er() thing, which slightly
complicates this; then suppose we schedule an event on RSP0, another on
RSP1, then remove the RSP0 one. Even in that case, the remaining RSP1
event will not change its constraint, since intel_fixup_er() rewrites
the event to be a native RSP1 event.

So that too reduces to the prior case.


That said; I have simplified the above condition to:

@@ -858,8 +858,17 @@ int x86_schedule_events(struct cpu_hw_ev
 		x86_pmu.start_scheduling(cpuc);
 
 	for (i = 0, wmin = X86_PMC_IDX_MAX, wmax = 0; i < n; i++) {
-		c = x86_pmu.get_event_constraints(cpuc, i, cpuc->event_list[i]);
-		cpuc->event_constraint[i] = c;
+		c = cpuc->event_constraint[i];
+
+		/*
+		 * Request constraints for new events; or for those events that
+		 * have a dynamic constraint -- for those the constraint can
+		 * change due to external factors (sibling state, allow_tfa).
+		 */
+		if (!c || (c->flags & PERF_X86_EVENT_DYNAMIC)) {
+			c = x86_pmu.get_event_constraints(cpuc, i, cpuc->event_list[i]);
+			cpuc->event_constraint[i] = c;
+		}
 
 		wmin = min(wmin, c->weight);
 		wmax = max(wmax, c->weight);

Because any dynamic event can change from one moment to the next.