Re: [PATCH v2 6/8] drivers/perf: hisi_pcie: Relax the check on related events

[Jonathan Cameron]

On Fri, 23 Feb 2024 18:33:57 +0800
Yicong Yang <yangyicong@xxxxxxxxxx> wrote:

> From: Junhao He <hejunhao3@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxx>

> ---
>  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;