Messages in this thread | | | From | Stephane Eranian <> | Date | Tue, 19 Mar 2019 10:52:01 -0700 | Subject | Re: [PATCH 1/8] perf/x86/intel: Fix memory corruption |
| |
On Tue, Mar 19, 2019 at 4:05 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Mon, Mar 18, 2019 at 11:29:25PM -0700, Stephane Eranian wrote: > > > > --- a/arch/x86/events/intel/core.c > > > +++ b/arch/x86/events/intel/core.c > > > @@ -3410,7 +3410,7 @@ tfa_get_event_constraints(struct cpu_hw_ > > > /* > > > * Without TFA we must not use PMC3. > > > */ > > > - if (!allow_tsx_force_abort && test_bit(3, c->idxmsk)) { > > > + if (!allow_tsx_force_abort && test_bit(3, c->idxmsk) && idx >= 0) { > > > c = dyn_constraint(cpuc, c, idx); > > > c->idxmsk64 &= ~(1ULL << 3); > > > c->weight--; > > > > > > > > > I was not cc'd on the patch that added allow_tsx_force_abort, so I > > Yeah, that never was public :-( I didn't particularly like that, but > that's the way it is. > > > will give some comments here. > > > If I understand the goal of the control parameter it is to turn on/off > > the TFA workaround and thus determine whether or not PMC3 is > > available. I don't know why you would need to make this a runtime > > tunable. > > Not quite; the control on its own doesn't directly write the MSR. And > even when the work-around is allowed, we'll not set the MSR unless there > is also demand for PMC3. > Trying to understand this better here. When the workaround is enabled (tfa=0), you lose PMC3 and transactions operate normally. When it is disabled (tfa=1), transactions are all aborted and PMC3 is available. If you are saying that when there is a PMU event requesting PMC3, then you need PMC3 avail, so you set the MSR so that tfa=1 forcing all transactions to abort. But in that case, you are modifying the execution of the workload when you are monitoring it, assuming it uses TSX. You want lowest overhead and no modifications to how the workload operates, otherwise how representative is the data you are collecting? I understand that there is no impact on apps not using TSX, well, except on context switch where you have to toggle that MSR. But for workloads using TSX, there is potentially an impact.
> It is a runtime tunable because boot parameters suck. > > > That seems a bit dodgy. But given the code you have here right now, we > > have to deal with it. A sysadmin could flip the control at any time, > > including when PMC3 is already in used by some events. I do not see > > the code that schedules out all the events on all CPUs once PMC3 > > becomes unavailable. You cannot just rely on the next context-switch > > or timer tick for multiplexing. > > Yeah, meh. You're admin, you can 'fix' it. In practise I don't expect > most people to care about the knob, and the few people that do, should > be able to make it work.
I don't understand how this can work reliably. You have a knob to toggle that MSR. Then, you have another one inside perf_events and then the sysadmin has to make sure nobody (incl. NMI watchdog) is using the PMU when this all happens. How can this be a practical solution? Am I missing something here?
| |