Received-SPF: pass (google.com: domain of linux-kernel+bounces-58040-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 8 Feb 2024 12:19:12 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Yicong Yang <yangyicong@huawei.com>
CC: <will@kernel.org>, <mark.rutland@arm.com>, <hejunhao3@huawei.com>,
	<linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<yangyicong@hisilicon.com>, <linuxarm@huawei.com>,
	<prime.zeng@hisilicon.com>, <fanghao11@huawei.com>
Subject: Re: [PATCH 2/7] drivers/perf: hisi_pcie: Fix incorrect counting
 under metric mode
Message-ID: <20240208121912.00001fd7@Huawei.com>
In-Reply-To: <20240204074527.47110-3-yangyicong@huawei.com>
References: <20240204074527.47110-1-yangyicong@huawei.com>
	<20240204074527.47110-3-yangyicong@huawei.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

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)