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, 15 Mar 2019 16:11:07 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     "Lendacky, Thomas" <Thomas.Lendacky@amd.com>
Cc:     "x86@kernel.org" <x86@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Namhyung Kim <namhyung@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jiri Olsa <jolsa@redhat.com>
Subject: Re: [RFC PATCH 2/2] x86/perf/amd: Resolve NMI latency issues when
 multiple PMCs are active
Message-ID: <20190315151107.GG6058@hirez.programming.kicks-ass.net>
References: <155232291547.21417.2499429555505085131.stgit@tlendack-t1.amdoffice.net>
 <155232292961.21417.3665243457569518550.stgit@tlendack-t1.amdoffice.net>
 <20190315120311.GX5996@hirez.programming.kicks-ass.net>
 <ef7fb4f8-29a8-16e6-5a8c-18da2eaf4243@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ef7fb4f8-29a8-16e6-5a8c-18da2eaf4243@amd.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 15, 2019 at 02:44:32PM +0000, Lendacky, Thomas wrote:

> >> @@ -689,6 +731,7 @@ static __initconst const struct x86_pmu amd_pmu = {
> >>   
> >>   	.amd_nb_constraints	= 1,
> >>   	.wait_on_overflow	= amd_pmu_wait_on_overflow,
> >> +	.mitigate_nmi_latency	= amd_pmu_mitigate_nmi_latency,
> >>   };
> > 
> > Again, you could just do amd_pmu_handle_irq() and avoid an extra
> > callback.
> 
> This is where there would be a bunch of code duplication where I thought
> adding the callback at the end would be better. But if it's best to add
> an AMD handle_irq callback I can do that. I'm easy, let me know if you'd
> prefer that.

Hmm, the thing that avoids you directly using x86_pmu_handle_irq() is
that added active count, but is that not the same as the POPCNT of
cpuc->active_mask?

Is the latency of POPCNT so bad that we need avoid it?

That is, I was thinking of something like:

int amd_pmu_handle_irq(struct pt_regs *regs)
{
	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
	int active = hweight_long(cpuc->active_mask);
	int handled = x86_pmu_handle_irq(regs);

+       if (active <= 1) {
		this_cpu_write(perf_nmi_counter, 0);
+               return handled;
	}
+
+       /*
+        * If a counter was handled, record the number of possible remaining
+        * NMIs that can occur.
+        */
+       if (handled) {
+               this_cpu_write(perf_nmi_counter,
+                              min_t(unsigned int, 2, active - 1));
+
+               return handled;
+       }
+
+       if (!this_cpu_read(perf_nmi_counter))
+               return NMI_DONE;
+
+       this_cpu_dec(perf_nmi_counter);
+
+       return NMI_HANDLED;
}

> > Anyway, we already had code to deal with spurious NMIs from AMD; see
> > commit:
> > 
> >    63e6be6d98e1 ("perf, x86: Catch spurious interrupts after disabling counters")
> > 
> > And that looks to be doing something very much the same. Why then do you
> > still need this on top?
> 
> This can happen while perf is handling normal counter overflow as opposed
> to covering the disabling of the counter case. When multiple counters
> overflow at roughly the same time, but the NMI doesn't arrive in time to
> get collapsed into a pending NMI, the back-to-back support in
> do_default_nmi() doesn't kick in.
> 
> Hmmm... I wonder if the wait on overflow in the disable_all() function
> would eliminate the need for 63e6be6d98e1. That would take a more testing
> on some older hardware to verify. That's something I can look into
> separate from this series.

Yes please, or at least better document the reason for their separate
existence. It's all turning into a bit of magic it seems.