Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 07 Nov 2022 15:39:07 +0000
Message-ID: <86y1smpyec.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Leo Yan <leo.yan@linaro.org>
Cc:     James Morse <james.morse@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        John Garry <john.garry@huawei.com>,
        James Clark <james.clark@arm.com>,
        Mike Leach <mike.leach@linaro.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
        kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org
Subject: Re: [PATCH v1 3/3] perf arm64: Support virtual CPU ID for kvm-stat
In-Reply-To: <Y2kabsQdddiX4G+O@leoy-huanghe.lan>
References: <20221105072311.8214-1-leo.yan@linaro.org>
        <20221105072311.8214-4-leo.yan@linaro.org>
        <868rkpr0mv.wl-maz@kernel.org>
        <Y2kabsQdddiX4G+O@leoy-huanghe.lan>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 07 Nov 2022 14:47:10 +0000,
Leo Yan <leo.yan@linaro.org> wrote:
> 
> On Sat, Nov 05, 2022 at 01:28:40PM +0000, Marc Zyngier wrote:
> 
> [...]
> 
> > > Before:
> > > 
> > >   # perf kvm stat report --vcpu 27
> > > 
> > >   Analyze events for all VMs, VCPU 27:
> > > 
> > >                VM-EXIT    Samples  Samples%     Time%    Min Time    Max Time         Avg time
> > > 
> > >   Total Samples:0, Total events handled time:0.00us.
> > >
> > > After:
> > > 
> > >   # perf kvm stat report --vcpu 27
> > > 
> > >   Analyze events for all VMs, VCPU 27:
> > > 
> > >                VM-EXIT    Samples  Samples%     Time%    Min Time    Max Time         Avg time
> > > 
> > >                  SYS64        808    98.54%    91.24%      0.00us    303.76us      3.46us ( +-  13.54% )
> > >                    WFx         10     1.22%     7.79%      0.00us     69.48us     23.91us ( +-  25.91% )
> > >                    IRQ          2     0.24%     0.97%      0.00us     22.64us     14.82us ( +-  52.77% )
> > > 
> > >   Total Samples:820, Total events handled time:3068.28us.
> > 
> > Please educate me: how useful is it to filter on a vcpu number across
> > all VMs? What sense does it even make?
> 
> Now "perf kvm" tool is not sophisticated since it doesn't capture VMID
> and virtual CPU ID together.

VMID is not a relevant indicator anyway, as it can change at any
point. But that's only to show that everybody has a different view on
what they need to collect. At which point, we need to provide an
infrastructure for data extraction, and not the data itself.

> I think a case is we can spin a program on a specific virtual CPU with
> taskset in VM, in this way we can check if any bottleneck is caused by
> VM entry/exit, but I have to say that it's inaccurate if we only filter
> on VCPU ID, we should consider tracing VMID and VCPU ID together in
> later's enhancement.
> 
> > Conversely, what would be the purpose of filtering on a 5th thread of
> > any process irrespective of what the process does? To me, this is the
> > same level of non-sense.
> 
> I agree.
> 
> > AFAICT, this is just piling more arbitrary data extraction for no
> > particular reason other than "just because we can", and there is
> > absolutely no guarantee that this is fit for anyone else's purpose.
> > 
> > I'd rather you have a generic tracepoint taking the vcpu as a context
> > and a BPF program that spits out the information people actually need,
> > keeping things out of the kernel. Or even a tracehook (like the
> > scheduler does), and let people load a module to dump whatever
> > information they please.
> 
> Actually I considered three options:
> 
> Option 1: Simply add new version's trace events for recording more info.
> This is not flexible and we even have risk to add more version's trace
> event if later we might find that more data should traced.
> 
> This approach is straightforward and the implementation would be
> simple.  This is main reason why finally I choosed to add new trace
> events.

But that doesn't scale at all.

> 
> Option 2: use Kprobe to dynamically insert tracepoints; but this means
> the user must have the corresponding vmlinux file, otherwise, perf
> tool might inject tracepoint at an incorrect address.  This is the
> main reason I didn't use Kprobe to add dynamic tracepoints.
>
> Option 3: As you suggested, I can bind KVM tracepoints with a eBPF
> program and the eBPF program records perf events.
> 
> When I reviewed Arm64's kvm_entry / kvm_exit trace events, they don't
> have vcpu context in the arguments, this means I need to add new trace
> events for accessing "vcpu" context.

I'm not opposed to adding new trace{point,hook}s if you demonstrate
that they are generic enough or will never need to evolve.

> 
> Option 1 and 3 both need to add trace events; option 1 is more
> straightforward solution and this is why it was choosed in current patch
> set.
> 
> I recognized that I made a mistake, actually we can modify the trace
> event's definition for kvm_entry / kvm_exit, note we only modify the
> trace event's arguments, this will change the trace function's
> definition but it will not break ABI (the format is exactly same for
> the user space).  Below changes demonstrate what's my proposing:
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 94d33e296e10..16f6b61abfec 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -917,7 +917,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>                 /**************************************************************
>                  * Enter the guest
>                  */
> -               trace_kvm_entry(*vcpu_pc(vcpu));
> +               trace_kvm_entry(vcpu);
>                 guest_timing_enter_irqoff();
>  
>                 ret = kvm_arm_vcpu_enter_exit(vcpu);
> diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h
> index 33e4e7dd2719..9df4fd30093c 100644
> --- a/arch/arm64/kvm/trace_arm.h
> +++ b/arch/arm64/kvm/trace_arm.h
> @@ -12,15 +12,15 @@
>   * Tracepoints for entry/exit to guest
>   */
>  TRACE_EVENT(kvm_entry,
> -       TP_PROTO(unsigned long vcpu_pc),
> -       TP_ARGS(vcpu_pc),
> +       TP_PROTO(struct kvm_vcpu *vcpu),
> +       TP_ARGS(vcpu),
>  
>         TP_STRUCT__entry(
>                 __field(        unsigned long,  vcpu_pc         )
>         ),
>  
>         TP_fast_assign(
> -               __entry->vcpu_pc                = vcpu_pc;
> +               __entry->vcpu_pc                = *vcpu_pc(vcpu);
>         ),
>  
>         TP_printk("PC: 0x%016lx", __entry->vcpu_pc)
> 
> Please let me know your opinion, if you don't object, I can move
> forward with this approach.

I have no issue with this if this doesn't change anything else.

And if you can make use of this with a BPF program and get to the same
result as your initial patch, then please submit it for inclusion in
the kernel as an example. We can then point people to it next time
this crop up (probably before Xmas).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.