[PATCH 0/3] powerpc/perf: Fix vpa-pmu driver to report correct

[PATCH 0/3] powerpc/perf: Fix vpa-pmu driver to report correct

[Gautam Menghani]

The vpa-pmu driver can sometimes report incorrect context switch latency
numbers:

$ perf stat -e vpa_pmu/l1_to_l2_lat/  --tid=2061  -I 1000
           time             counts unit events
     1.001022517 18,446,744,073,708,177,408      vpa_pmu/l1_to_l2_lat/
     2.002120674            345,092      vpa_pmu/l1_to_l2_lat/
     3.003171582            406,342      vpa_pmu/l1_to_l2_lat/
     4.004234838            340,836      vpa_pmu/l1_to_l2_lat/

These invalid numbers are observed because the vpa-pmu driver does not
handle the following scenarios:

1. The vCPU process gets rescheduled to a different host cpu.

2. The vCPU process is not running, and vpa_pmu_read() is called to read
counter values.

Fix the above cases by maintaining aggregate counter values inside the
vcpu->arch struct and using them to report data.

Gautam Menghani (3):
  KVM: PPC: Book3S HV: Maintain aggregate context switch values for
    vcpus
  powerpc/perf: Use the aggregate context switch values from vcpu struct
  powerpc/perf: Update prev_count of event to get accurate values

 arch/powerpc/include/asm/kvm_book3s_64.h |  6 --
 arch/powerpc/kvm/book3s_hv.c             | 69 +----------------------
 arch/powerpc/perf/vpa-pmu.c              | 70 ++++++++++++------------
 3 files changed, 39 insertions(+), 106 deletions(-)

-- 
2.54.0

[PATCH 1/3] KVM: PPC: Book3S HV: Maintain aggregate context switch values for vCPUs

[Gautam Menghani]

The variables in vcpu->arch currently store the latest values read from
PACA when the vCPU exits. Change this to instead store aggregate values
so that every vCPU has its own record of total context switch latencies
it has accumulated. This is essential for the vpa-pmu driver to report
correct numbers.

Signed-off-by: Gautam Menghani <gautam@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 3cfe9a7be9c6..0e8a959122a8 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -4164,12 +4164,12 @@ static void do_trace_nested_cs_time(struct kvm_vcpu *vcpu)
 	trace_kvmppc_vcpu_stats(vcpu, l1_to_l2_ns - *l1_to_l2_cs_ptr,
 					l2_to_l1_ns - *l2_to_l1_cs_ptr,
 					l2_runtime_ns - *l2_runtime_agg_ptr);
+	vcpu->arch.l1_to_l2_cs += (l1_to_l2_ns - *l1_to_l2_cs_ptr);
+	vcpu->arch.l2_to_l1_cs += (l2_to_l1_ns - *l2_to_l1_cs_ptr);
+	vcpu->arch.l2_runtime_agg += (l2_runtime_ns - *l2_runtime_agg_ptr);
 	*l1_to_l2_cs_ptr = l1_to_l2_ns;
 	*l2_to_l1_cs_ptr = l2_to_l1_ns;
 	*l2_runtime_agg_ptr = l2_runtime_ns;
-	vcpu->arch.l1_to_l2_cs = l1_to_l2_ns;
-	vcpu->arch.l2_to_l1_cs = l2_to_l1_ns;
-	vcpu->arch.l2_runtime_agg = l2_runtime_ns;
 }
 
 u64 kvmhv_get_l1_to_l2_cs_time(void)
-- 
2.54.0

[PATCH 2/3] powerpc/perf: Use the aggregate context switch values from vcpu struct

[Gautam Menghani]

The vpa-pmu driver reports incorrect numbers in 2 scenarios:

1. The vCPU process gets rescheduled to a different host cpu
    - Incorrect numbers are observed here because the PACA is per-host cpu
      resource, and the KVM vCPUs can be rescheduled to different
      host cpus. This causes the vpa-pmu driver to subtract wrong values
      when vCPUs are rescheduled.

2. The vCPU is not running when vpa_pmu_read() is called.
    - In this case get_counter_data() returns 0, and this can result in
      negative numbers getting reported.

Fix the above issues by using the aggregate values from the vcpu
structure to capture and report the difference in counter values.

Signed-off-by: Gautam Menghani <gautam@linux.ibm.com>
---
 arch/powerpc/include/asm/kvm_book3s_64.h |  6 ---
 arch/powerpc/kvm/book3s_hv.c             | 63 ----------------------
 arch/powerpc/perf/vpa-pmu.c              | 69 ++++++++++++------------
 3 files changed, 35 insertions(+), 103 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index b936e174eefd..11065313d4c1 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -688,12 +688,6 @@ int kvmhv_counters_tracepoint_regfunc(void);
 void kvmhv_counters_tracepoint_unregfunc(void);
 int kvmhv_get_l2_counters_status(void);
 void kvmhv_set_l2_counters_status(int cpu, bool status);
-u64 kvmhv_get_l1_to_l2_cs_time(void);
-u64 kvmhv_get_l2_to_l1_cs_time(void);
-u64 kvmhv_get_l2_runtime_agg(void);
-u64 kvmhv_get_l1_to_l2_cs_time_vcpu(void);
-u64 kvmhv_get_l2_to_l1_cs_time_vcpu(void);
-u64 kvmhv_get_l2_runtime_agg_vcpu(void);
 
 #endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
 
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 0e8a959122a8..b315a959d2a1 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -4172,69 +4172,6 @@ static void do_trace_nested_cs_time(struct kvm_vcpu *vcpu)
 	*l2_runtime_agg_ptr = l2_runtime_ns;
 }
 
-u64 kvmhv_get_l1_to_l2_cs_time(void)
-{
-	return tb_to_ns(be64_to_cpu(get_lppaca()->l1_to_l2_cs_tb));
-}
-EXPORT_SYMBOL(kvmhv_get_l1_to_l2_cs_time);
-
-u64 kvmhv_get_l2_to_l1_cs_time(void)
-{
-	return tb_to_ns(be64_to_cpu(get_lppaca()->l2_to_l1_cs_tb));
-}
-EXPORT_SYMBOL(kvmhv_get_l2_to_l1_cs_time);
-
-u64 kvmhv_get_l2_runtime_agg(void)
-{
-	return tb_to_ns(be64_to_cpu(get_lppaca()->l2_runtime_tb));
-}
-EXPORT_SYMBOL(kvmhv_get_l2_runtime_agg);
-
-u64 kvmhv_get_l1_to_l2_cs_time_vcpu(void)
-{
-	struct kvm_vcpu *vcpu;
-	struct kvm_vcpu_arch *arch;
-
-	vcpu = local_paca->kvm_hstate.kvm_vcpu;
-	if (vcpu) {
-		arch = &vcpu->arch;
-		return arch->l1_to_l2_cs;
-	} else {
-		return 0;
-	}
-}
-EXPORT_SYMBOL(kvmhv_get_l1_to_l2_cs_time_vcpu);
-
-u64 kvmhv_get_l2_to_l1_cs_time_vcpu(void)
-{
-	struct kvm_vcpu *vcpu;
-	struct kvm_vcpu_arch *arch;
-
-	vcpu = local_paca->kvm_hstate.kvm_vcpu;
-	if (vcpu) {
-		arch = &vcpu->arch;
-		return arch->l2_to_l1_cs;
-	} else {
-		return 0;
-	}
-}
-EXPORT_SYMBOL(kvmhv_get_l2_to_l1_cs_time_vcpu);
-
-u64 kvmhv_get_l2_runtime_agg_vcpu(void)
-{
-	struct kvm_vcpu *vcpu;
-	struct kvm_vcpu_arch *arch;
-
-	vcpu = local_paca->kvm_hstate.kvm_vcpu;
-	if (vcpu) {
-		arch = &vcpu->arch;
-		return arch->l2_runtime_agg;
-	} else {
-		return 0;
-	}
-}
-EXPORT_SYMBOL(kvmhv_get_l2_runtime_agg_vcpu);
-
 #else
 int kvmhv_get_l2_counters_status(void)
 {
diff --git a/arch/powerpc/perf/vpa-pmu.c b/arch/powerpc/perf/vpa-pmu.c
index bff4cfab7b94..334ef7719db9 100644
--- a/arch/powerpc/perf/vpa-pmu.c
+++ b/arch/powerpc/perf/vpa-pmu.c
@@ -71,6 +71,28 @@ static const struct attribute_group *vpa_pmu_attr_groups[] = {
 	NULL
 };
 
+static u64 get_vcpu_data(struct kvm_vcpu *vcpu, u64 config)
+{
+	u64 new_data;
+
+	if (!vcpu)
+		return 0;
+
+	switch (config) {
+	case L1_TO_L2_CS_LAT:
+		new_data = vcpu->arch.l1_to_l2_cs;
+		break;
+	case L2_TO_L1_CS_LAT:
+		new_data = vcpu->arch.l2_to_l1_cs;
+		break;
+	case L2_RUNTIME_AGG:
+		new_data = vcpu->arch.l2_runtime_agg;
+		break;
+	}
+
+	return new_data;
+}
+
 static int vpa_pmu_event_init(struct perf_event *event)
 {
 	if (event->attr.type != event->pmu->type)
@@ -91,56 +113,35 @@ static int vpa_pmu_event_init(struct perf_event *event)
 	return 0;
 }
 
-static unsigned long get_counter_data(struct perf_event *event)
-{
-	unsigned int config = event->attr.config;
-	u64 data;
-
-	switch (config) {
-	case L1_TO_L2_CS_LAT:
-		if (event->attach_state & PERF_ATTACH_TASK)
-			data = kvmhv_get_l1_to_l2_cs_time_vcpu();
-		else
-			data = kvmhv_get_l1_to_l2_cs_time();
-		break;
-	case L2_TO_L1_CS_LAT:
-		if (event->attach_state & PERF_ATTACH_TASK)
-			data = kvmhv_get_l2_to_l1_cs_time_vcpu();
-		else
-			data = kvmhv_get_l2_to_l1_cs_time();
-		break;
-	case L2_RUNTIME_AGG:
-		if (event->attach_state & PERF_ATTACH_TASK)
-			data = kvmhv_get_l2_runtime_agg_vcpu();
-		else
-			data = kvmhv_get_l2_runtime_agg();
-		break;
-	default:
-		data = 0;
-		break;
-	}
-
-	return data;
-}
-
 static int vpa_pmu_add(struct perf_event *event, int flags)
 {
 	u64 data;
+	struct kvm_vcpu *vcpu;
+
+	vcpu = local_paca->kvm_hstate.kvm_vcpu;
+	if (!vcpu)
+		goto out;
 
+	event->pmu_private = vcpu;
 	kvmhv_set_l2_counters_status(smp_processor_id(), true);
 
-	data = get_counter_data(event);
+	data = get_vcpu_data(vcpu, event->attr.config);
 	local64_set(&event->hw.prev_count, data);
 
+out:
 	return 0;
 }
 
 static void vpa_pmu_read(struct perf_event *event)
 {
 	u64 prev_data, new_data, final_data;
+	struct kvm_vcpu *vcpu;
 
+	vcpu = (struct kvm_vcpu *) event->pmu_private;
+	if (!vcpu)
+		return;
 	prev_data = local64_read(&event->hw.prev_count);
-	new_data = get_counter_data(event);
+	new_data = get_vcpu_data(vcpu, event->attr.config);
 	final_data = new_data - prev_data;
 
 	local64_add(final_data, &event->count);
-- 
2.54.0

[PATCH 3/3] powerpc/perf: Update prev_count of event to get accurate values

[Gautam Menghani]

When vpa_pmu_read() is called twice while a process is running on a cpu,
the "event->hw.prev_count" variable returns a stale value on the 2nd
read which results in a larger value getting added to event->count.

Fix this by updating the event->hw.prev_count every time vpa_pmu_read()
is called.

Signed-off-by: Gautam Menghani <gautam@linux.ibm.com>
---
 arch/powerpc/perf/vpa-pmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/perf/vpa-pmu.c b/arch/powerpc/perf/vpa-pmu.c
index 334ef7719db9..f5cfa4d5e369 100644
--- a/arch/powerpc/perf/vpa-pmu.c
+++ b/arch/powerpc/perf/vpa-pmu.c
@@ -145,6 +145,7 @@ static void vpa_pmu_read(struct perf_event *event)
 	final_data = new_data - prev_data;
 
 	local64_add(final_data, &event->count);
+	local64_set(&event->hw.prev_count, new_data);
 }
 
 static void vpa_pmu_del(struct perf_event *event, int flags)
-- 
2.54.0