diff mbox series

[6/7] drivers/perf: hisi_pcie: Merge find_related_event() and get_event_idx()

Message ID 20240204074527.47110-7-yangyicong@huawei.com (mailing list archive)
State New, archived
Headers show
Series Several updates for HiSilicon PCIe PMU driver | expand

Commit Message

Yicong Yang Feb. 4, 2024, 7:45 a.m. UTC 
From: Junhao He <hejunhao3@huawei.com>

The function xxx_find_related_event() scan all working events to find
related events. During this process, we also can find the idle counters.
If not found related events, return the first idle counter to simplify
the code.

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 55 ++++++++++----------------
 1 file changed, 21 insertions(+), 34 deletions(-)

Comments

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

> From: Junhao He <hejunhao3@huawei.com>
> 
> The function xxx_find_related_event() scan all working events to find
> related events. During this process, we also can find the idle counters.
> If not found related events, return the first idle counter to simplify
> the code.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

A suggestion inline to avoid the magic HISI_PCIE_MAX_COUNTER value
being used outside of the function. 

> ---
>  drivers/perf/hisilicon/hisi_pcie_pmu.c | 55 ++++++++++----------------
>  1 file changed, 21 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> index 1b45aeb82859..2edde66675e7 100644
> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> @@ -397,16 +397,24 @@ static u64 hisi_pcie_pmu_read_counter(struct perf_event *event)
>  	return hisi_pcie_pmu_readq(pcie_pmu, event->hw.event_base, idx);
>  }
>  
> -static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
> -					    struct perf_event *event)
> +/*
> + * Check all work events, if a relevant event is found then we return it
> + * first, otherwise return the first idle counter (need to reset).
> + */
> +static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu,
> +					struct perf_event *event)
>  {
> +	int first_idle = HISI_PCIE_MAX_COUNTERS;
	int first_idle = -EAGAIN;
>  	struct perf_event *sibling;
>  	int idx;
>  
>  	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
>  		sibling = pcie_pmu->hw_events[idx];
> -		if (!sibling)
> +		if (!sibling) {
> +			if (first_idle == HISI_PCIE_MAX_COUNTERS)
if (first_idle == -EAGAIN)

> +				first_idle = idx;
>  			continue;
> +		}
>  
>  		/* Related events must be used in group */
>  		if (hisi_pcie_pmu_cmp_event(sibling, event) &&
> @@ -414,19 +422,7 @@ static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
>  			return idx;
>  	}
>  
> -	return idx;
> -}
> -
> -static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu)
> -{
> -	int idx;
> -
> -	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
> -		if (!pcie_pmu->hw_events[idx])
> -			return idx;
> -	}
> -
> -	return -EINVAL;
> +	return first_idle;
Then this will return -EAGAIN;
>  }
>  
>  static void hisi_pcie_pmu_event_update(struct perf_event *event)
> @@ -552,27 +548,18 @@ static int hisi_pcie_pmu_add(struct perf_event *event, int flags)
>  
>  	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
>  
> -	/* Check all working events to find a related event. */
> -	idx = hisi_pcie_pmu_find_related_event(pcie_pmu, event);
> -	if (idx < 0)
> -		return idx;
> -
> -	/* Current event shares an enabled counter with the related event */
> -	if (idx < HISI_PCIE_MAX_COUNTERS) {
> -		hwc->idx = idx;
> -		goto start_count;
> -	}
> -
> -	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu);
> -	if (idx < 0)
> -		return idx;
> +	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu, event);
> +	if (idx == HISI_PCIE_MAX_COUNTERS)
> +		return -EAGAIN;

Perhaps simpler to handle first_idle == HISI_PCIE_MAX_COUNTERS as
an error return in hisi_pcie_pmu_get_event_idx - see above.

 	if (idx < 0)
		return idx;


>  
>  	hwc->idx = idx;
> -	pcie_pmu->hw_events[idx] = event;
> -	/* Reset Counter to avoid previous statistic interference. */
> -	hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
>  
> -start_count:
> +	/* No enabled counter found with related event, reset it */
> +	if (!pcie_pmu->hw_events[idx]) {
> +		hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
> +		pcie_pmu->hw_events[idx] = event;
> +	}
> +
>  	if (flags & PERF_EF_START)
>  		hisi_pcie_pmu_start(event, PERF_EF_RELOAD);
>
Junhao He Feb. 22, 2024, 3 a.m. UTC | #2 
Hi, Jonathan


On 2024/2/8 20:39, Jonathan Cameron wrote:
> On Sun, 4 Feb 2024 15:45:26 +0800
> Yicong Yang <yangyicong@huawei.com> wrote:
>
>> From: Junhao He <hejunhao3@huawei.com>
>>
>> The function xxx_find_related_event() scan all working events to find
>> related events. During this process, we also can find the idle counters.
>> If not found related events, return the first idle counter to simplify
>> the code.
>>
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> A suggestion inline to avoid the magic HISI_PCIE_MAX_COUNTER value
> being used outside of the function.

Thanks for the comments, will fix in next version.

>
>> ---
>>   drivers/perf/hisilicon/hisi_pcie_pmu.c | 55 ++++++++++----------------
>>   1 file changed, 21 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
>> index 1b45aeb82859..2edde66675e7 100644
>> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
>> @@ -397,16 +397,24 @@ static u64 hisi_pcie_pmu_read_counter(struct perf_event *event)
>>   	return hisi_pcie_pmu_readq(pcie_pmu, event->hw.event_base, idx);
>>   }
>>   
>> -static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
>> -					    struct perf_event *event)
>> +/*
>> + * Check all work events, if a relevant event is found then we return it
>> + * first, otherwise return the first idle counter (need to reset).
>> + */
>> +static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu,
>> +					struct perf_event *event)
>>   {
>> +	int first_idle = HISI_PCIE_MAX_COUNTERS;
> 	int first_idle = -EAGAIN;

Yes, I will do that.

>>   	struct perf_event *sibling;
>>   	int idx;
>>   
>>   	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
>>   		sibling = pcie_pmu->hw_events[idx];
>> -		if (!sibling)
>> +		if (!sibling) {
>> +			if (first_idle == HISI_PCIE_MAX_COUNTERS)
> if (first_idle == -EAGAIN)

Ok, will fix it.

>
>> +				first_idle = idx;
>>   			continue;
>> +		}
>>   
>>   		/* Related events must be used in group */
>>   		if (hisi_pcie_pmu_cmp_event(sibling, event) &&
>> @@ -414,19 +422,7 @@ static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
>>   			return idx;
>>   	}
>>   
>> -	return idx;
>> -}
>> -
>> -static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu)
>> -{
>> -	int idx;
>> -
>> -	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
>> -		if (!pcie_pmu->hw_events[idx])
>> -			return idx;
>> -	}
>> -
>> -	return -EINVAL;
>> +	return first_idle;
> Then this will return -EAGAIN;

Okay

>>   }
>>   
>>   static void hisi_pcie_pmu_event_update(struct perf_event *event)
>> @@ -552,27 +548,18 @@ static int hisi_pcie_pmu_add(struct perf_event *event, int flags)
>>   
>>   	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
>>   
>> -	/* Check all working events to find a related event. */
>> -	idx = hisi_pcie_pmu_find_related_event(pcie_pmu, event);
>> -	if (idx < 0)
>> -		return idx;
>> -
>> -	/* Current event shares an enabled counter with the related event */
>> -	if (idx < HISI_PCIE_MAX_COUNTERS) {
>> -		hwc->idx = idx;
>> -		goto start_count;
>> -	}
>> -
>> -	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu);
>> -	if (idx < 0)
>> -		return idx;
>> +	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu, event);
>> +	if (idx == HISI_PCIE_MAX_COUNTERS)
>> +		return -EAGAIN;
> Perhaps simpler to handle first_idle == HISI_PCIE_MAX_COUNTERS as
> an error return in hisi_pcie_pmu_get_event_idx - see above.
>
>   	if (idx < 0)
> 		return idx;

Sure.

>
>>   
>>   	hwc->idx = idx;
>> -	pcie_pmu->hw_events[idx] = event;
>> -	/* Reset Counter to avoid previous statistic interference. */
>> -	hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
>>   
>> -start_count:
>> +	/* No enabled counter found with related event, reset it */
>> +	if (!pcie_pmu->hw_events[idx]) {
>> +		hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
>> +		pcie_pmu->hw_events[idx] = event;
>> +	}
>> +
>>   	if (flags & PERF_EF_START)
>>   		hisi_pcie_pmu_start(event, PERF_EF_RELOAD);
>>   
>
diff mbox series

Patch

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 1b45aeb82859..2edde66675e7 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -397,16 +397,24 @@  static u64 hisi_pcie_pmu_read_counter(struct perf_event *event)
 	return hisi_pcie_pmu_readq(pcie_pmu, event->hw.event_base, idx);
 }
 
-static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
-					    struct perf_event *event)
+/*
+ * Check all work events, if a relevant event is found then we return it
+ * first, otherwise return the first idle counter (need to reset).
+ */
+static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu,
+					struct perf_event *event)
 {
+	int first_idle = HISI_PCIE_MAX_COUNTERS;
 	struct perf_event *sibling;
 	int idx;
 
 	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
 		sibling = pcie_pmu->hw_events[idx];
-		if (!sibling)
+		if (!sibling) {
+			if (first_idle == HISI_PCIE_MAX_COUNTERS)
+				first_idle = idx;
 			continue;
+		}
 
 		/* Related events must be used in group */
 		if (hisi_pcie_pmu_cmp_event(sibling, event) &&
@@ -414,19 +422,7 @@  static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
 			return idx;
 	}
 
-	return idx;
-}
-
-static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu)
-{
-	int idx;
-
-	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
-		if (!pcie_pmu->hw_events[idx])
-			return idx;
-	}
-
-	return -EINVAL;
+	return first_idle;
 }
 
 static void hisi_pcie_pmu_event_update(struct perf_event *event)
@@ -552,27 +548,18 @@  static int hisi_pcie_pmu_add(struct perf_event *event, int flags)
 
 	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
 
-	/* Check all working events to find a related event. */
-	idx = hisi_pcie_pmu_find_related_event(pcie_pmu, event);
-	if (idx < 0)
-		return idx;
-
-	/* Current event shares an enabled counter with the related event */
-	if (idx < HISI_PCIE_MAX_COUNTERS) {
-		hwc->idx = idx;
-		goto start_count;
-	}
-
-	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu);
-	if (idx < 0)
-		return idx;
+	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu, event);
+	if (idx == HISI_PCIE_MAX_COUNTERS)
+		return -EAGAIN;
 
 	hwc->idx = idx;
-	pcie_pmu->hw_events[idx] = event;
-	/* Reset Counter to avoid previous statistic interference. */
-	hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
 
-start_count:
+	/* No enabled counter found with related event, reset it */
+	if (!pcie_pmu->hw_events[idx]) {
+		hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
+		pcie_pmu->hw_events[idx] = event;
+	}
+
 	if (flags & PERF_EF_START)
 		hisi_pcie_pmu_start(event, PERF_EF_RELOAD);