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

[Peter Zijlstra]

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@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx>

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.