Received-SPF: pass (google.com: domain of linux-kernel+bounces-81778-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 26 Feb 2024 15:16:52 +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 v2 6/8] drivers/perf: hisi_pcie: Relax the check on
 related events
Message-ID: <20240226151652.00004e88@Huawei.com>
In-Reply-To: <20240223103359.18669-7-yangyicong@huawei.com>
References: <20240223103359.18669-1-yangyicong@huawei.com>
	<20240223103359.18669-7-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 Fri, 23 Feb 2024 18:33:57 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> If we use two events with the same filter and related event type
> (see the following example), the driver check whether they are related
> events and are in the same group, otherwise the function
> hisi_pcie_pmu_find_related_event() return -EINVAL, then the 2nd event
> cannot count but the 1st event is running, although the PCIe PMU has
> other idle counters.
> 
> In this case, The perf event scheduler will make the two events to
> multiplex a counter, if the user use the formula
> (1st event_value / 2nd event_value) to calculate the bandwidth, he/she
> won't get the correct value, because they are not counting at the
> same period.
> 
> This patch tries to fix this by making the related events to use
> different idle counters if they are not in the same event group.
> 
> And finally, I'm going to say. The related events are best used in the
> same group [1]. There are two ways to know if they are related events.
> a) By event name, such as the latency events "xxx_latency, xxx_cnt" or
> bandwidth events "xxx_flux, xxx_time".
> b) By event type, such as "event=0xXXXX, event=0x1XXXX".
> 
> Use group to count the related events:
>   [1] -e "{pmu_name/xxx_latency,port=1/,pmu_name/xxx_cnt,port=1/}"
> 
>   example:
>     1st event: hisi_pcie0_core1/event=0x804,port=1
>     2nd event: hisi_pcie0_core1/event=0x10804,port=1
> 
>   test cmd:
>     perf stat -e hisi_pcie0_core1/event=0x804,port=1/ \
>                -e hisi_pcie0_core1/event=0x10804,port=1/
> 
>   before patch:
>             25,281      hisi_pcie0_core1/event=0x804,port=1/    (49.91%)
>            470,598      hisi_pcie0_core1/event=0x10804,port=1/    (50.09%)
> 
>   after patch:
>             24,147      hisi_pcie0_core1/event=0x804,port=1/
>            474,558      hisi_pcie0_core1/event=0x10804,port=1/
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>

> ---
>  drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> index b2dde7559639..5b15f3698188 100644
> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> @@ -409,14 +409,10 @@ static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
>  		if (!sibling)
>  			continue;
>  
> -		if (!hisi_pcie_pmu_cmp_event(sibling, event))
> -			continue;
> -
>  		/* Related events must be used in group */
> -		if (sibling->group_leader == event->group_leader)
> +		if (hisi_pcie_pmu_cmp_event(sibling, event) &&
> +		    sibling->group_leader == event->group_leader)
>  			return idx;
> -		else
> -			return -EINVAL;
>  	}
>  
>  	return idx;