MIME-Version: 1.0
References: <20211111084415.663951-1-eranian@google.com> <20211111084415.663951-4-eranian@google.com>
 <YY6QBXs0sM16DdbV@hirez.programming.kicks-ass.net> <CABPqkBShSBaJH+PR6rMkRRzjZAKN5zPhcdnLWx=4a-yQWxcA2A@mail.gmail.com>
 <20211116082923.GX174703@worktop.programming.kicks-ass.net>
 <CABPqkBQ4BCswvNPpkO79dBamhudikz1cGCXFpwAp9xsTb3F8xQ@mail.gmail.com>
 <YZZE+bPCokVrTARM@hirez.programming.kicks-ass.net> <YZZH+5odIawPQtgJ@hirez.programming.kicks-ass.net>
In-Reply-To: <YZZH+5odIawPQtgJ@hirez.programming.kicks-ass.net>
From:   Stephane Eranian <eranian@google.com>
Date:   Mon, 29 Nov 2021 14:07:58 -0800
Message-ID: <CABPqkBSLHS82MM9f9jcLNfYDAR7+j4h2ztm22wjMH11=FM8FcA@mail.gmail.com>
Subject: Re: [PATCH v2 03/13] perf/x86/amd: add AMD Fam19h Branch Sampling support
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     linux-kernel@vger.kernel.org, kim.phillips@amd.com,
        acme@redhat.com, jolsa@redhat.com, songliubraving@fb.com,
        mpe@ellerman.id.au, maddy@linux.ibm.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Peter,

Back from a vacation break. Comments below.

On Thu, Nov 18, 2021 at 4:33 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Nov 18, 2021 at 01:20:09PM +0100, Peter Zijlstra wrote:
> > On Tue, Nov 16, 2021 at 11:23:39PM -0800, Stephane Eranian wrote:
>
> > > Ok, I made the changes you suggested. It looks closer to the way LBR is handled.
> > > However, this means that there is no path by which you can get to
> > > amd_pmu_disable_event()
> > > without having gone through amd_pmu_disable_all(). Is that always the
> > > case? And same thing
> > > on the enable side.
> >
> > So that's true for ->add() and ->del(), those cannot be called without
> > being wrapped in ->pmu_disable(), ->pmu_enable().
> >
> > There is however the ->stop() and ->start() usage for throttling, which
> > can stop an individual event (while leaving the event scheduled on the
> > PMU). Now, I think the ->stop() gets called with the PMU enabled, but
> > the ->start() is with it disabled again.
>
> I just looked, and the throttling depends on the PMU's PMI handler
> implementation, for Intel it will have the PMU disabled, for generic and
> AMD it has it enabled (see x86_pmu_handle_irq -- also these are really
> NMIs but lets not do a mass rename just now).
>
Yes, I see that too. It has to do with the __perf_event_overflow()
-> __perf_event_account_interrupt() code path which does not call
perf_pmu_disable().
And that's because it knows it is called from PMI and let's the PMI
code decide the state
of the PMU. Unfortunately, on AMD, the PMU is not stopped fully on PMI
and that is because
there is no centralized way of doing this, so you'd have 6 wrmsr x 2
to disable/enable. OTOH,
I don't see the point of monitoring in the PMI code. So there is a
discrepancy between Intel and
AMD here. I think we should fix it.

> > The ramification would be that we'd stop the event, but leave BRS
> > enabled for a throttled event. Which should be harmless, no?

Well, the risk here is that if BRS is left enabled, it may hold the
NMI for up to 16 taken branches.
If the associated event is disabled, then cpuc->events[idx] = NULL.
The BRS drain function checks
for that and does not capture any sample. The brs_drain() function is
called from the PMI handler if
cpuc->lbr_users > 0 which would be the case on the stop() path. I
think this is the only risk we have
on the throttling code path.

There would be no problem if we were to stop/start the PMU in the AMD
PMI handler.

Thanks.