linux-kernel - Re: [PATCH 2/7] drivers/perf: hisi_pcie: Fix incorrect counting under metric mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240208121912.00001fd7@Huawei.com>
Date: Thu, 8 Feb 2024 12:19:12 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Yicong Yang <yangyicong@...wei.com>
CC: <will@...nel.org>, <mark.rutland@....com>, <hejunhao3@...wei.com>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<yangyicong@...ilicon.com>, <linuxarm@...wei.com>,
	<prime.zeng@...ilicon.com>, <fanghao11@...wei.com>
Subject: Re: [PATCH 2/7] drivers/perf: hisi_pcie: Fix incorrect counting
 under metric mode

On Sun, 4 Feb 2024 15:45:22 +0800
Yicong Yang <yangyicong@...wei.com> wrote:

> From: Yicong Yang <yangyicong@...ilicon.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@...alhost 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@...alhost 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@...ilicon.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)