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:   Wed, 09 Aug 2023 09:48:29 +0100
Message-ID: <864jl8ha8y.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Huang Shijie <shijie@os.amperecomputing.com>
Cc:     oliver.upton@linux.dev, james.morse@arm.com,
        suzuki.poulose@arm.com, yuzenghui@huawei.com,
        catalin.marinas@arm.com, will@kernel.org, pbonzini@redhat.com,
        peterz@infradead.org, ingo@redhat.com, acme@kernel.org,
        mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
        jolsa@kernel.org, namhyung@kernel.org, irogers@google.com,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-perf-users@vger.kernel.org, patches@amperecomputing.com,
        zwang@amperecomputing.com
Subject: Re: [PATCH] perf/core: fix the bug in the event multiplexing
In-Reply-To: <20230809013953.7692-1-shijie@os.amperecomputing.com>
References: <20230809013953.7692-1-shijie@os.amperecomputing.com>
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/28.2
 (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 Wed, 09 Aug 2023 02:39:53 +0100,
Huang Shijie <shijie@os.amperecomputing.com> wrote:

For a start, please provide a sensible subject line for your patch.
"fix the bug" is not exactly descriptive, and I'd argue that if there
was only one bug left, I'd have taken an early retirement by now.

> 
> 1.) Background.
>    1.1) In arm64, run a virtual guest with Qemu, and bind the guest

Is that with QEMU in system emulation mode? Or QEMU as a VMM for KVM?

>         to core 33 and run program "a" in guest.

Is core 33 significant? Is the program itself significant?

>         The code of "a" shows below:
>    	----------------------------------------------------------
> 		#include <stdio.h>
> 
> 		int main()
> 		{
> 			unsigned long i = 0;
> 
> 			for (;;) {
> 				i++;
> 			}
> 
> 			printf("i:%ld\n", i);
> 			return 0;
> 		}
>    	----------------------------------------------------------
> 
>    1.2) Use the following perf command in host:
>       #perf stat -e cycles:G,cycles:H -C 33 -I 1000 sleep 1
>           #           time             counts unit events
>                1.000817400      3,299,471,572      cycles:G
>                1.000817400          3,240,586      cycles:H
> 
>        This result is correct, my cpu's frequency is 3.3G.
> 
>    1.3) Use the following perf command in host:
>       #perf stat -e cycles:G,cycles:H -C 33 -d -d  -I 1000 sleep 1
>             time             counts unit events
>      1.000831480        153,634,097      cycles:G                                                                (70.03%)
>      1.000831480      3,147,940,599      cycles:H                                                                (70.03%)
>      1.000831480      1,143,598,527      L1-dcache-loads                                                         (70.03%)
>      1.000831480              9,986      L1-dcache-load-misses            #    0.00% of all L1-dcache accesses   (70.03%)
>      1.000831480    <not supported>      LLC-loads
>      1.000831480    <not supported>      LLC-load-misses
>      1.000831480        580,887,696      L1-icache-loads                                                         (70.03%)
>      1.000831480             77,855      L1-icache-load-misses            #    0.01% of all L1-icache accesses   (70.03%)
>      1.000831480      6,112,224,612      dTLB-loads                                                              (70.03%)
>      1.000831480             16,222      dTLB-load-misses                 #    0.00% of all dTLB cache accesses  (69.94%)
>      1.000831480        590,015,996      iTLB-loads                                                              (59.95%)
>      1.000831480                505      iTLB-load-misses                 #    0.00% of all iTLB cache accesses  (59.95%)
> 
>        This result is wrong. The "cycle:G" should be nearly 3.3G.
> 
> 2.) Root cause.
> 	There is only 7 counters in my arm64 platform:
> 	  (one cycle counter) + (6 normal counters)
> 
> 	In 1.3 above, we will use 10 event counters.
> 	Since we only have 7 counters, the perf core will trigger
>        	event multiplexing in hrtimer:
> 	     merge_sched_in() -->perf_mux_hrtimer_restart() -->
> 	     perf_rotate_context().
> 
>        In the perf_rotate_context(), it does not restore some PMU registers
>        as context_switch() does.  In context_switch():
>              kvm_sched_in()  --> kvm_vcpu_pmu_restore_guest()
>              kvm_sched_out() --> kvm_vcpu_pmu_restore_host()
> 
>        So we got wrong result.
> 
> 3.) About this patch.
>         3.1) Add arch_perf_rotate_pmu_set()
>         3.2) Add is_guest().
> 	     Check the context for hrtimer.
> 	3.3) In arm64's arch_perf_rotate_pmu_set(),
>        	     set the PMU registers by the context.
> 
> 4.) Test result of this patch:
>       #perf stat -e cycles:G,cycles:H -C 33 -d -d  -I 1000 sleep 1
>             time             counts unit events
>      1.000817360      3,297,898,244      cycles:G                                                                (70.03%)
>      1.000817360          2,719,941      cycles:H                                                                (70.03%)
>      1.000817360            883,764      L1-dcache-loads                                                         (70.03%)
>      1.000817360             17,517      L1-dcache-load-misses            #    1.98% of all L1-dcache accesses   (70.03%)
>      1.000817360    <not supported>      LLC-loads
>      1.000817360    <not supported>      LLC-load-misses
>      1.000817360          1,033,816      L1-icache-loads                                                         (70.03%)
>      1.000817360            103,839      L1-icache-load-misses            #   10.04% of all L1-icache accesses   (70.03%)
>      1.000817360            982,401      dTLB-loads                                                              (70.03%)
>      1.000817360             28,272      dTLB-load-misses                 #    2.88% of all dTLB cache accesses  (69.94%)
>      1.000817360            972,072      iTLB-loads                                                              (59.95%)
>      1.000817360                772      iTLB-load-misses                 #    0.08% of all iTLB cache accesses  (59.95%)
> 
>     The result is correct. The "cycle:G" is nearly 3.3G now.
> 
> Signed-off-by: Huang Shijie <shijie@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/pmu.c     | 8 ++++++++
>  include/linux/kvm_host.h | 1 +
>  kernel/events/core.c     | 5 +++++
>  virt/kvm/kvm_main.c      | 9 +++++++++
>  4 files changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index 121f1a14c829..a6815c3f0c4e 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c
> @@ -210,6 +210,14 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
>  	kvm_vcpu_pmu_disable_el0(events_guest);
>  }
>  
> +void arch_perf_rotate_pmu_set(void)
> +{
> +	if (is_guest())
> +		kvm_vcpu_pmu_restore_guest(NULL);
> +	else
> +		kvm_vcpu_pmu_restore_host(NULL);
> +}

So we're now randomly poking at the counters even when no guest is
running, based on whatever is stashed in internal KVM data structures?
I'm sure this is going to work really well.

Hint: even if these functions don't directly look at the vcpu pointer,
passing NULL is a really bad idea. It is a sure sign that you don't
have the context on which to perform the action you're trying to do.

This really shouldn't do *anything* when the rotation process is not
preempting a guest.

> +
>  /*
>   * With VHE, keep track of the PMUSERENR_EL0 value for the host EL0 on the pCPU
>   * where PMUSERENR_EL0 for the guest is loaded, since PMUSERENR_EL0 is switched
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 9d3ac7720da9..e350cbc8190f 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -931,6 +931,7 @@ void kvm_destroy_vcpus(struct kvm *kvm);
>  
>  void vcpu_load(struct kvm_vcpu *vcpu);
>  void vcpu_put(struct kvm_vcpu *vcpu);
> +bool is_guest(void);

Why do we need this (not to mention the poor choice of name)? We
already have kvm_get_running_vcpu(), which does everything you need
(and gives you the actual context).

>  
>  #ifdef __KVM_HAVE_IOAPIC
>  void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6fd9272eec6e..fe78f9d17eba 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4229,6 +4229,10 @@ ctx_event_to_rotate(struct perf_event_pmu_context *pmu_ctx)
>  	return event;
>  }
>  
> +void __weak arch_perf_rotate_pmu_set(void)
> +{
> +}
> +
>  static bool perf_rotate_context(struct perf_cpu_pmu_context *cpc)
>  {
>  	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
> @@ -4282,6 +4286,7 @@ static bool perf_rotate_context(struct perf_cpu_pmu_context *cpc)
>  	if (task_event || (task_epc && cpu_event))
>  		__pmu_ctx_sched_in(task_epc->ctx, pmu);
>  
> +	arch_perf_rotate_pmu_set();

KVM already supports hooking into the perf core using the
perf_guest_info_callbacks structure. Why should we need a separate
mechanism?

>  	perf_pmu_enable(pmu);
>  	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
>  
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index dfbaafbe3a00..a77d336552be 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -218,6 +218,15 @@ void vcpu_load(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(vcpu_load);
>  
> +/* Do we in the guest? */
> +bool is_guest(void)
> +{
> +	struct kvm_vcpu *vcpu;
> +
> +	vcpu = __this_cpu_read(kvm_running_vcpu);
> +	return !!vcpu;
> +}
> +
>  void vcpu_put(struct kvm_vcpu *vcpu)
>  {
>  	preempt_disable();

It looks like you've identified an actual issue. However, I'm highly
sceptical of the implementation. This really needs some more work.

Another question is how the same thing is handled on x86? Maybe they
don't suffer from this problem thanks to specific architectural
features, but it'd be good to find out, as this may guide the
implementation in a different way.

Thanks,

	M.

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