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:   Sat, 05 Nov 2022 13:28:40 +0000
Message-ID: <868rkpr0mv.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: <20221105072311.8214-4-leo.yan@linaro.org>
References: <20221105072311.8214-1-leo.yan@linaro.org>
        <20221105072311.8214-4-leo.yan@linaro.org>
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 Sat, 05 Nov 2022 07:23:11 +0000,
Leo Yan <leo.yan@linaro.org> wrote:
> 
> Since the two trace events kvm_entry_v2/kvm_exit_v2 are added, we can
> use the field "vcpu_id" in the events to get to know the virtual CPU ID.
> To keep backward compatibility, we still need to rely on the trace
> events kvm_entry/kvm_exit for old kernels.
> 
> This patch adds Arm64's functions setup_kvm_events_tp() and
> arm64__setup_kvm_tp(), by detecting the nodes under sysfs folder, it can
> dynamically register trace events kvm_entry_v2/kvm_exit_v2 when the
> kernel has provided them, otherwise, it rolls back to use events
> kvm_entry/kvm_exit for backward compatibility.
> 
> Let cpu_isa_init() to invoke arm64__setup_kvm_tp(), this can allow the
> command "perf kvm stat report" also to dynamically setup trace events.
> 
> 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?

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.

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.

But randomly adding new tracepoints to output a semi-useless field
without any consideration for future-proofing? No, thank you.

Thanks,

	M.

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