1. Patchew
  2. linux
  3. [RESEND PATCH v12 0/3] Add perf support for HiSilicon PCIe Tune and Trace device
  4. View patch

[RESEND PATCH v12 1/3] perf tool: arm: Refactor event list iteration in auxtrace_record__init()

Yicong Yang posted 3 patches 3 years, 6 months ago
There is a newer version of this series
[RESEND PATCH v12 1/3] perf tool: arm: Refactor event list iteration in auxtrace_record__init()
Posted by Yicong Yang 3 years, 6 months ago 
From: Qi Liu <liuqi115@huawei.com>

Add find_pmu_for_event() and use to simplify logic in
auxtrace_record_init(). find_pmu_for_event() will be
reused in subsequent patches.

Reviewed-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Qi Liu <liuqi115@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 tools/perf/arch/arm/util/auxtrace.c | 53 ++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
index 5fc6a2a3dbc5..384c7cfda0fd 100644
--- a/tools/perf/arch/arm/util/auxtrace.c
+++ b/tools/perf/arch/arm/util/auxtrace.c
@@ -50,16 +50,32 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
 	return arm_spe_pmus;
 }
 
+static struct perf_pmu *find_pmu_for_event(struct perf_pmu **pmus,
+					   int pmu_nr, struct evsel *evsel)
+{
+	int i;
+
+	if (!pmus)
+		return NULL;
+
+	for (i = 0; i < pmu_nr; i++) {
+		if (evsel->core.attr.type == pmus[i]->type)
+			return pmus[i];
+	}
+
+	return NULL;
+}
+
 struct auxtrace_record
 *auxtrace_record__init(struct evlist *evlist, int *err)
 {
-	struct perf_pmu	*cs_etm_pmu;
+	struct perf_pmu	*cs_etm_pmu = NULL;
+	struct perf_pmu **arm_spe_pmus = NULL;
 	struct evsel *evsel;
-	bool found_etm = false;
+	struct perf_pmu *found_etm = NULL;
 	struct perf_pmu *found_spe = NULL;
-	struct perf_pmu **arm_spe_pmus = NULL;
+	int auxtrace_event_cnt = 0;
 	int nr_spes = 0;
-	int i = 0;
 
 	if (!evlist)
 		return NULL;
@@ -68,24 +84,23 @@ struct auxtrace_record
 	arm_spe_pmus = find_all_arm_spe_pmus(&nr_spes, err);
 
 	evlist__for_each_entry(evlist, evsel) {
-		if (cs_etm_pmu &&
-		    evsel->core.attr.type == cs_etm_pmu->type)
-			found_etm = true;
-
-		if (!nr_spes || found_spe)
-			continue;
-
-		for (i = 0; i < nr_spes; i++) {
-			if (evsel->core.attr.type == arm_spe_pmus[i]->type) {
-				found_spe = arm_spe_pmus[i];
-				break;
-			}
-		}
+		if (cs_etm_pmu && !found_etm)
+			found_etm = find_pmu_for_event(&cs_etm_pmu, 1, evsel);
+
+		if (arm_spe_pmus && !found_spe)
+			found_spe = find_pmu_for_event(arm_spe_pmus, nr_spes, evsel);
 	}
+
 	free(arm_spe_pmus);
 
-	if (found_etm && found_spe) {
-		pr_err("Concurrent ARM Coresight ETM and SPE operation not currently supported\n");
+	if (found_etm)
+		auxtrace_event_cnt++;
+
+	if (found_spe)
+		auxtrace_event_cnt++;
+
+	if (auxtrace_event_cnt > 1) {
+		pr_err("Concurrent AUX trace operation not currently supported\n");
 		*err = -EOPNOTSUPP;
 		return NULL;
 	}
-- 
2.24.0
Re: [RESEND PATCH v12 1/3] perf tool: arm: Refactor event list iteration in auxtrace_record__init()
Posted by John Garry 3 years, 6 months ago 
On 14/09/2022 08:59, Yicong Yang wrote:
> From: Qi Liu <liuqi115@huawei.com>
> 
> Add find_pmu_for_event() and use to simplify logic in
> auxtrace_record_init(). find_pmu_for_event() will be
> reused in subsequent patches.
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>   tools/perf/arch/arm/util/auxtrace.c | 53 ++++++++++++++++++-----------
>   1 file changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
> index 5fc6a2a3dbc5..384c7cfda0fd 100644
> --- a/tools/perf/arch/arm/util/auxtrace.c
> +++ b/tools/perf/arch/arm/util/auxtrace.c
> @@ -50,16 +50,32 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
>   	return arm_spe_pmus;
>   }
>   
> +static struct perf_pmu *find_pmu_for_event(struct perf_pmu **pmus,
> +					   int pmu_nr, struct evsel *evsel)
> +{
> +	int i;
> +
> +	if (!pmus)
> +		return NULL;
> +
> +	for (i = 0; i < pmu_nr; i++) {
> +		if (evsel->core.attr.type == pmus[i]->type)
> +			return pmus[i];
> +	}
> +
> +	return NULL;
> +}
> +
>   struct auxtrace_record
>   *auxtrace_record__init(struct evlist *evlist, int *err)
>   {
> -	struct perf_pmu	*cs_etm_pmu;
> +	struct perf_pmu	*cs_etm_pmu = NULL;
> +	struct perf_pmu **arm_spe_pmus = NULL;
>   	struct evsel *evsel;
> -	bool found_etm = false;
> +	struct perf_pmu *found_etm = NULL;
>   	struct perf_pmu *found_spe = NULL;
> -	struct perf_pmu **arm_spe_pmus = NULL;
> +	int auxtrace_event_cnt = 0;
>   	int nr_spes = 0;
> -	int i = 0;
>   
>   	if (!evlist)
>   		return NULL;
> @@ -68,24 +84,23 @@ struct auxtrace_record
>   	arm_spe_pmus = find_all_arm_spe_pmus(&nr_spes, err);
>   
>   	evlist__for_each_entry(evlist, evsel) {
> -		if (cs_etm_pmu &&
> -		    evsel->core.attr.type == cs_etm_pmu->type)
> -			found_etm = true;
> -
> -		if (!nr_spes || found_spe)
> -			continue;
> -
> -		for (i = 0; i < nr_spes; i++) {
> -			if (evsel->core.attr.type == arm_spe_pmus[i]->type) {
> -				found_spe = arm_spe_pmus[i];
> -				break;
> -			}
> -		}
> +		if (cs_etm_pmu && !found_etm) 
> +			found_etm = find_pmu_for_event(&cs_etm_pmu, 1, evsel);
> +
> +		if (arm_spe_pmus && !found_spe)
> +			found_spe = find_pmu_for_event(arm_spe_pmus, nr_spes, evsel);

should you break if found_etm and found_spe are set? Or, indeed, error 
and return directly as we do below? Indeed, I am not sure why you even 
require auxtrace_event_cnt

>   	}
> +
>   	free(arm_spe_pmus);
>   
> -	if (found_etm && found_spe) {
> -		pr_err("Concurrent ARM Coresight ETM and SPE operation not currently supported\n");
> +	if (found_etm)
> +		auxtrace_event_cnt++;
> +
> +	if (found_spe)
> +		auxtrace_event_cnt++;
> +
> +	if (auxtrace_event_cnt > 1) {
> +		pr_err("Concurrent AUX trace operation not currently supported\n");
>   		*err = -EOPNOTSUPP;
>   		return NULL;
>   	}
Re: [RESEND PATCH v12 1/3] perf tool: arm: Refactor event list iteration in auxtrace_record__init()
Posted by Leo Yan 3 years, 6 months ago 
On Wed, Sep 14, 2022 at 02:47:43PM +0100, John Garry wrote:

[...]

> >   struct auxtrace_record
> >   *auxtrace_record__init(struct evlist *evlist, int *err)
> >   {
> > -	struct perf_pmu	*cs_etm_pmu;
> > +	struct perf_pmu	*cs_etm_pmu = NULL;
> > +	struct perf_pmu **arm_spe_pmus = NULL;
> >   	struct evsel *evsel;
> > -	bool found_etm = false;
> > +	struct perf_pmu *found_etm = NULL;
> >   	struct perf_pmu *found_spe = NULL;
> > -	struct perf_pmu **arm_spe_pmus = NULL;
> > +	int auxtrace_event_cnt = 0;
> >   	int nr_spes = 0;
> > -	int i = 0;
> >   	if (!evlist)
> >   		return NULL;
> > @@ -68,24 +84,23 @@ struct auxtrace_record
> >   	arm_spe_pmus = find_all_arm_spe_pmus(&nr_spes, err);
> >   	evlist__for_each_entry(evlist, evsel) {
> > -		if (cs_etm_pmu &&
> > -		    evsel->core.attr.type == cs_etm_pmu->type)
> > -			found_etm = true;
> > -
> > -		if (!nr_spes || found_spe)
> > -			continue;
> > -
> > -		for (i = 0; i < nr_spes; i++) {
> > -			if (evsel->core.attr.type == arm_spe_pmus[i]->type) {
> > -				found_spe = arm_spe_pmus[i];
> > -				break;
> > -			}
> > -		}
> > +		if (cs_etm_pmu && !found_etm) +			found_etm =
> > find_pmu_for_event(&cs_etm_pmu, 1, evsel);
> > +
> > +		if (arm_spe_pmus && !found_spe)
> > +			found_spe = find_pmu_for_event(arm_spe_pmus, nr_spes, evsel);
> 
> should you break if found_etm and found_spe are set? Or, indeed, error and
> return directly as we do below? Indeed, I am not sure why you even require
> auxtrace_event_cnt

I think this was my suggestion :)

We can check if both 'found_etm' and 'found_spe' are set and directly
break (and bail out) for this case.  But it would introduce more complex
checking if we connect with patch 2 with new flag 'found_ptt', something
like:

  if ((found_etm && found_spe) ||
      (found_etm && found_ptt) ||
      (found_spe && found_ptt))
      break;

This is hard for later's extension if we need to support a new auxtrace
event, so using auxtrace_event_cnt would be easier to extend more
auxtrace event on Arm platforms.

Thanks,
Leo

> >   	}
> > +
> >   	free(arm_spe_pmus);
> > -	if (found_etm && found_spe) {
> > -		pr_err("Concurrent ARM Coresight ETM and SPE operation not currently supported\n");
> > +	if (found_etm)
> > +		auxtrace_event_cnt++;
> > +
> > +	if (found_spe)
> > +		auxtrace_event_cnt++;
> > +
> > +	if (auxtrace_event_cnt > 1) {
> > +		pr_err("Concurrent AUX trace operation not currently supported\n");
> >   		*err = -EOPNOTSUPP;
> >   		return NULL;
> >   	}
>
Re: [RESEND PATCH v12 1/3] perf tool: arm: Refactor event list iteration in auxtrace_record__init()
Posted by Yicong Yang 3 years, 6 months ago 
On 2022/9/14 22:27, Leo Yan wrote:
> On Wed, Sep 14, 2022 at 02:47:43PM +0100, John Garry wrote:
> 
> [...]
> 
>>>   struct auxtrace_record
>>>   *auxtrace_record__init(struct evlist *evlist, int *err)
>>>   {
>>> -	struct perf_pmu	*cs_etm_pmu;
>>> +	struct perf_pmu	*cs_etm_pmu = NULL;
>>> +	struct perf_pmu **arm_spe_pmus = NULL;
>>>   	struct evsel *evsel;
>>> -	bool found_etm = false;
>>> +	struct perf_pmu *found_etm = NULL;
>>>   	struct perf_pmu *found_spe = NULL;
>>> -	struct perf_pmu **arm_spe_pmus = NULL;
>>> +	int auxtrace_event_cnt = 0;
>>>   	int nr_spes = 0;
>>> -	int i = 0;
>>>   	if (!evlist)
>>>   		return NULL;
>>> @@ -68,24 +84,23 @@ struct auxtrace_record
>>>   	arm_spe_pmus = find_all_arm_spe_pmus(&nr_spes, err);
>>>   	evlist__for_each_entry(evlist, evsel) {
>>> -		if (cs_etm_pmu &&
>>> -		    evsel->core.attr.type == cs_etm_pmu->type)
>>> -			found_etm = true;
>>> -
>>> -		if (!nr_spes || found_spe)
>>> -			continue;
>>> -
>>> -		for (i = 0; i < nr_spes; i++) {
>>> -			if (evsel->core.attr.type == arm_spe_pmus[i]->type) {
>>> -				found_spe = arm_spe_pmus[i];
>>> -				break;
>>> -			}
>>> -		}
>>> +		if (cs_etm_pmu && !found_etm) +			found_etm =
>>> find_pmu_for_event(&cs_etm_pmu, 1, evsel);
>>> +
>>> +		if (arm_spe_pmus && !found_spe)
>>> +			found_spe = find_pmu_for_event(arm_spe_pmus, nr_spes, evsel);
>>
>> should you break if found_etm and found_spe are set? Or, indeed, error and
>> return directly as we do below? Indeed, I am not sure why you even require
>> auxtrace_event_cnt
> 
> I think this was my suggestion :)
> 

yes. thanks :). It's dicussed in v7 and for more information:
https://lore.kernel.org/lkml/20220430073411.GA657977@leoy-ThinkPad-X240s/

> We can check if both 'found_etm' and 'found_spe' are set and directly
> break (and bail out) for this case.  But it would introduce more complex
> checking if we connect with patch 2 with new flag 'found_ptt', something
> like:
> 
>   if ((found_etm && found_spe) ||
>       (found_etm && found_ptt) ||
>       (found_spe && found_ptt))
>       break;
> 
> This is hard for later's extension if we need to support a new auxtrace
> event, so using auxtrace_event_cnt would be easier to extend more
> auxtrace event on Arm platforms.
> 
> Thanks,
> Leo
> 
>>>   	}
>>> +
>>>   	free(arm_spe_pmus);
>>> -	if (found_etm && found_spe) {
>>> -		pr_err("Concurrent ARM Coresight ETM and SPE operation not currently supported\n");
>>> +	if (found_etm)
>>> +		auxtrace_event_cnt++;
>>> +
>>> +	if (found_spe)
>>> +		auxtrace_event_cnt++;
>>> +
>>> +	if (auxtrace_event_cnt > 1) {
>>> +		pr_err("Concurrent AUX trace operation not currently supported\n");
>>>   		*err = -EOPNOTSUPP;
>>>   		return NULL;
>>>   	}
>>
> .
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.