lkml.org 
[lkml]   [2019]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/8] perf/x86/intel: Fix memory corruption
    On Tue, Mar 19, 2019 at 11:20 AM Peter Zijlstra <peterz@infradead.org> wrote:
    >
    > On Tue, Mar 19, 2019 at 10:52:01AM -0700, Stephane Eranian wrote:
    > > > 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.
    >
    > Right, but we don't expose tfa.
    >
    > > 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.
    >
    > Right, so when allow_tfa=1 (default), we only set tfa=1 when PMC3 is
    > requested.
    >
    > This has the advantage that,
    >
    > TSX only workload -> works
    > perf 4 counteres -> works
    >
    > Only when you need both of them, do you get 'trouble'.
    >
    > > But in that case, you are modifying the execution of the workload when
    > > you are monitoring it, assuming it uses TSX.
    >
    > We assume you are not in fact using TSX, not a lot of code does. If you
    > do use TSX a lot, and you don't want to interfere, you have to set
    > allow_tfa=0 and live with one counter less.
    >
    > Any which way around you turn this stone, it sucks.
    >
    > > You want lowest overhead and no modifications to how the workload
    > > operates, otherwise how representative is the data you are collecting?
    >
    > Sure; but there are no good choices here. This 'fix' will break
    > something. We figured TSX+4-counter-perf was the least common scenario.
    >
    > We konw of people that rely on 4 counter being present; you want to
    > explain to them how when doing an update their program suddently doesn't
    > work anymore?
    >
    > Or you want to default to tfa=1; but then you have to explain to those
    > people relying on TSX why their workload stopped working.
    >
    > > I understand that there is no impact on apps not using TSX, well,
    > > except on context switch where you have to toggle that MSR.
    >
    > There is no additional code in the context switch; only the perf event
    > scheduling code prods at the MSR.
    >
    > > But for workloads using TSX, there is potentially an impact.
    >
    > Yes, well, if you're a TSX _and_ perf user, you now have an extra knob
    > to play with; that's not something I can do anything about. We're forced
    > to make a choice here.
    >
    > > > 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.
    >
    > No, we don't have this knob.
    >
    > > Then, you have another one inside perf_events
    >
    > Only this knob exists allow_tfa.
    >
    > > and then the sysadmin has to make sure nobody (incl. NMI watchdog) is
    > > using the PMU when this all happens.
    >
    > You're very unlucky if the watchdog runs on PMC3, normally it runs on
    > Fixed1 or something. Esp early after boot. (Remember, we schedule fixed
    > counters first, and then general purpose counters, with a preference for
    > lower counters).
    >
    > Anyway, you can trivially switch it off if you want.
    >
    > > How can this be a practical solution? Am I missing something here?
    >
    > It works just fine; it is unfortunate that we have this interaction but
    > that's not something we can do anything about. We're forced to deal with
    > this.
    >
    Right now, if I do:

    echo 0 > /sys/bus/event_source/devices/cpu/allow_tsx_force_abort

    Then I don't have the guarantee on when there will be no abort when I
    return from the echo.
    the MSR is accessed only on PMU scheduling. I would expect a sysadmin
    to want some guarantee
    if this is to be switched on/off at runtime. If not, then having a
    boot time option is better in my opinion.

    This other bit I noticed is that cpuc->tfa_shadow is used to avoid the
    wrmsr(), but I don't see the
    code that makes sure the init value (0) matches the value of the MSR.
    Is this MSR guarantee to be
    zero on reset? How about on kexec()?

    > But if you're a TSX+perf user, have your boot scripts do:
    >
    > echo 0 > /sys/bus/event_source/devices/cpu/allow_tsx_force_abort
    >
    > and you'll not use PMC3 and TSX will be 'awesome'. If you don't give a
    > crap about TSX (most people), just boot and be happy.
    >
    > If you do care about TSX+perf and want to dynamically toggle for some
    > reason, you just have to be a little careful.

    \
     
     \ /
      Last update: 2019-03-20 21:48    [W:2.381 / U:0.264 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site