diff mbox series

[2/7] drivers/perf: hisi_pcie: Fix incorrect counting under metric mode

Message ID 20240204074527.47110-3-yangyicong@huawei.com
State New
Headers
Series None |

Commit Message

Yicong Yang Feb. 4, 2024, 7:45 a.m. UTC 
  From: Yicong Yang <yangyicong@hisilicon.com>

The metric counting shows incorrect results if the events in the
metric group using the same event but different filter options.
This is because we only judge the event code to decide whether
the event in the metric group should share the same hardware
counter, but ignore the settings of the filter.

For example, on a platform of 2 ports 0x1 and 0x2 but only port
0x1 has a downstream PCIe NVME device. The metric counting
shows both ports have the same counts because we misassign these
two events to one same hardware counter:
[root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'

 Performance counter stats for 'system wide':

        7907484924      hisi_pcie0_core1/event=0x0104,port=0x2/
        7907484924      hisi_pcie0_core1/event=0x0104,port=0x1/

      10.153863691 seconds time elapsed

Fix this by using the whole config rather than the event only
to judge whether two events are the same and should share the
same hardware counter. With this patch, the metric counting in
the above case tends to be corrected:

[root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'

 Performance counter stats for 'system wide':

                 0      hisi_pcie0_core1/event=0x0104,port=0x2/
        8123122077      hisi_pcie0_core1/event=0x0104,port=0x1/

      10.152875631 seconds time elapsed

Fixes: 8404b0fbc7fb ("drivers/perf: hisi: Add driver for HiSilicon PCIe PMU")
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Jonathan Cameron Feb. 8, 2024, 12:19 p.m. UTC | #1 
On Sun, 4 Feb 2024 15:45:22 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> The metric counting shows incorrect results if the events in the
> metric group using the same event but different filter options.
> This is because we only judge the event code to decide whether
> the event in the metric group should share the same hardware
> counter, but ignore the settings of the filter.
> 
> For example, on a platform of 2 ports 0x1 and 0x2 but only port
> 0x1 has a downstream PCIe NVME device. The metric counting
> shows both ports have the same counts because we misassign these
> two events to one same hardware counter:
> [root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'
> 
>  Performance counter stats for 'system wide':
> 
>         7907484924      hisi_pcie0_core1/event=0x0104,port=0x2/
>         7907484924      hisi_pcie0_core1/event=0x0104,port=0x1/
> 
>       10.153863691 seconds time elapsed
> 
> Fix this by using the whole config rather than the event only
> to judge whether two events are the same and should share the
> same hardware counter. With this patch, the metric counting in
> the above case tends to be corrected:
> 
> [root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'
> 
>  Performance counter stats for 'system wide':
> 
>                  0      hisi_pcie0_core1/event=0x0104,port=0x2/
>         8123122077      hisi_pcie0_core1/event=0x0104,port=0x1/
> 
>       10.152875631 seconds time elapsed
> 
> Fixes: 8404b0fbc7fb ("drivers/perf: hisi: Add driver for HiSilicon PCIe PMU")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/perf/hisilicon/hisi_pcie_pmu.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> index 11a819cd07f2..9623bed93876 100644
> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> @@ -314,10 +314,15 @@ static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
>  	return true;
>  }
>  
> +/*
> + * Check Whether two events share the same config. The same config means not
> + * only the event code, but also the filter settings of the two events are
> + * the same.
> + */
>  static bool hisi_pcie_pmu_cmp_event(struct perf_event *target,
>  					struct perf_event *event)
>  {
> -	return hisi_pcie_get_real_event(target) == hisi_pcie_get_real_event(event);
> +	return hisi_pcie_pmu_get_filter(target) == hisi_pcie_pmu_get_filter(event);

This is why I looked more closely at what get_filter() was doing in previous patch.
Not obvious from the naming here that this compares the event + filters rather than
now just comparing the filters.

>  }
>  
>  static bool hisi_pcie_pmu_validate_event_group(struct perf_event *event)
diff mbox series

Patch

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 11a819cd07f2..9623bed93876 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -314,10 +314,15 @@  static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
 	return true;
 }
 
+/*
+ * Check Whether two events share the same config. The same config means not
+ * only the event code, but also the filter settings of the two events are
+ * the same.
+ */
 static bool hisi_pcie_pmu_cmp_event(struct perf_event *target,
 					struct perf_event *event)
 {
-	return hisi_pcie_get_real_event(target) == hisi_pcie_get_real_event(event);
+	return hisi_pcie_pmu_get_filter(target) == hisi_pcie_pmu_get_filter(event);
 }
 
 static bool hisi_pcie_pmu_validate_event_group(struct perf_event *event)