Re: [PATCH v6 02/16] perf/x86/intel: Handle guest PEBS overflow PMI for KVM guest

From: Peter Zijlstra <peterz@infradead.org>
To: "Xu, Like" <like.xu@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	Sean Christopherson <seanjc@google.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	weijiang.yang@intel.com, Kan Liang <kan.liang@linux.intel.com>,
	ak@linux.intel.com, wei.w.wang@intel.com, eranian@google.com,
	liuxiangdong5@huawei.com, linux-kernel@vger.kernel.org,
	x86@kernel.org, kvm@vger.kernel.org,
	Like Xu <like.xu@linux.intel.com>
Subject: Re: [PATCH v6 02/16] perf/x86/intel: Handle guest PEBS overflow PMI for KVM guest
Date: Tue, 18 May 2021 10:37:46 +0200	[thread overview]
Message-ID: <YKN82utjfLEX9ZJh@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <bd684011-b83d-5c83-bdfb-926d6bc4595a@intel.com>

On Tue, May 18, 2021 at 03:38:52PM +0800, Xu, Like wrote:

> > I'm thinking you have your conditions in the wrong order; would it not
> > be much cheaper to first check: '!x86_pmu.pebs_active || !guest_pebs_idx'
> > than to do that horrible indirect ->is_in_guest() call?
> > 
> > After all, if the guest doesn't have PEBS enabled, who cares if we're
> > currently in a guest or not.
> 
> Yes, it makes sense. How about:
> 
> @@ -2833,6 +2867,10 @@ static int handle_pmi_common(struct pt_regs *regs,
> u64 status)
>                 u64 pebs_enabled = cpuc->pebs_enabled;
> 
>                 handled++;
> +               if (x86_pmu.pebs_vmx && x86_pmu.pebs_active &&
> +                   (cpuc->pebs_enabled & ~cpuc->intel_ctrl_host_mask) &&
> +                   (static_call(x86_guest_state)() & PERF_GUEST_ACTIVE))
> +                       x86_pmu_handle_guest_pebs(regs, &data);

This is terruble, just call x86_pmu_handle_guest_pebs() unconditionally
and put all the ugly inside it.

>                 x86_pmu.drain_pebs(regs, &data);
>                 status &= intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI;
> 
> > 
> > Also, something like the below perhaps (arm64 and xen need fixing up at
> > the very least) could make all that perf_guest_cbs stuff suck less.
> 
> How about the commit message for your below patch:
> 
> From: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> 
> x86/core: Use static_call to rewrite perf_guest_info_callbacks
> 
> The two fields named "is_in_guest" and "is_user_mode" in
> perf_guest_info_callbacks are replaced with a new multiplexed member
> named "state", and the "get_guest_ip" field will be renamed to "get_ip".
> 
> The application of DEFINE_STATIC_CALL_RET0 (arm64 and xen need fixing
> up at the very least) could make all that perf_guest_cbs stuff suck less.
> For KVM, these callbacks will be updated in the kvm_arch_init().
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Well, you *do* need to fix up arm64 and xen, we can't very well break
their builds can we now.

> ----
> 
> I'm not sue if you have a strong reason to violate the check-patch rule:
> 
> ERROR: Using weak declarations can have unintended link defects
> #238: FILE: include/linux/perf_event.h:1242:
> +extern void __weak arch_perf_update_guest_cbs(void);

Copy/paste fail I think. I didn't really put much effort into the patch,
only made sure defconfig+kvm_guest.config compiled.