1. Patchew
  2. linux
  3. [PATCH v1 00/10] Better fixes for grouping of events
  4. View patch

[PATCH v1 05/10] perf evsel: Limit in group test to CPUs

Ian Rogers posted 10 patches 3 years, 1 month ago
There is a newer version of this series
[PATCH v1 05/10] perf evsel: Limit in group test to CPUs
Posted by Ian Rogers 3 years, 1 month ago 
Don't just match on the event name, restict based on the PMU too.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/evsel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
index ea3972d785d1..580b0a172136 100644
--- a/tools/perf/arch/x86/util/evsel.c
+++ b/tools/perf/arch/x86/util/evsel.c
@@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
 	if (!evsel__sys_has_perf_metrics(evsel))
 		return false;
 
+	if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
+		return false;
+
 	return evsel->name &&
 		(strcasestr(evsel->name, "slots") ||
 		 strcasestr(evsel->name, "topdown"));
-- 
2.39.2.722.g9855ee24e9-goog
Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs
Posted by Liang, Kan 3 years, 1 month ago 

On 2023-03-01 11:12 p.m., Ian Rogers wrote:
> Don't just match on the event name, restict based on the PMU too.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/evsel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index ea3972d785d1..580b0a172136 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>  	if (!evsel__sys_has_perf_metrics(evsel))
>  		return false;
>  
> +	if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
> +		return false;

I'm not sure why we want to check the pmu name. It seems better to move
it into evsel__sys_has_perf_metrics(), since perf_metrics is a core PMU
only feature.

I think the strncmp(evsel->pmu_name, "cpu", 3) is to check whether it is
a core PMU. It is also used in other places. I think it's better to
factor out it, e.g., arch_evsel__is_core_pmu(). It will deliver a clear
message of what we are doing here.

Thanks,
Kan
> +
>  	return evsel->name &&
>  		(strcasestr(evsel->name, "slots") ||
>  		 strcasestr(evsel->name, "topdown"));
Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs
Posted by Ian Rogers 3 years, 1 month ago 
On Thu, Mar 2, 2023 at 7:24 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2023-03-01 11:12 p.m., Ian Rogers wrote:
> > Don't just match on the event name, restict based on the PMU too.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/x86/util/evsel.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> > index ea3972d785d1..580b0a172136 100644
> > --- a/tools/perf/arch/x86/util/evsel.c
> > +++ b/tools/perf/arch/x86/util/evsel.c
> > @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> >       if (!evsel__sys_has_perf_metrics(evsel))
> >               return false;
> >
> > +     if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
> > +             return false;
>
> I'm not sure why we want to check the pmu name. It seems better to move
> it into evsel__sys_has_perf_metrics(), since perf_metrics is a core PMU
> only feature.
>
> I think the strncmp(evsel->pmu_name, "cpu", 3) is to check whether it is
> a core PMU. It is also used in other places. I think it's better to
> factor out it, e.g., arch_evsel__is_core_pmu(). It will deliver a clear
> message of what we are doing here.
>
> Thanks,
> Kan

I missed the behavior of evsel__sys_has_perf_metrics and think we can
just drop this change. We should probably rename
evsel__sys_has_perf_metrics perhaps something like
arch_evsel__pmu_has_topdown_events.

Thanks,
Ian

> > +
> >       return evsel->name &&
> >               (strcasestr(evsel->name, "slots") ||
> >                strcasestr(evsel->name, "topdown"));
Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs
Posted by Liang, Kan 3 years, 1 month ago 

On 2023-03-02 2:38 p.m., Ian Rogers wrote:
> On Thu, Mar 2, 2023 at 7:24 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2023-03-01 11:12 p.m., Ian Rogers wrote:
>>> Don't just match on the event name, restict based on the PMU too.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>  tools/perf/arch/x86/util/evsel.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>>> index ea3972d785d1..580b0a172136 100644
>>> --- a/tools/perf/arch/x86/util/evsel.c
>>> +++ b/tools/perf/arch/x86/util/evsel.c
>>> @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>>       if (!evsel__sys_has_perf_metrics(evsel))
>>>               return false;
>>>
>>> +     if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
>>> +             return false;
>>
>> I'm not sure why we want to check the pmu name. It seems better to move
>> it into evsel__sys_has_perf_metrics(), since perf_metrics is a core PMU
>> only feature.
>>
>> I think the strncmp(evsel->pmu_name, "cpu", 3) is to check whether it is
>> a core PMU. It is also used in other places. I think it's better to
>> factor out it, e.g., arch_evsel__is_core_pmu(). It will deliver a clear
>> message of what we are doing here.
>>
>> Thanks,
>> Kan
> 
> I missed the behavior of evsel__sys_has_perf_metrics and think we can
> just drop this change.

Yes, dropping the change is also OK for me.

> We should probably rename
> evsel__sys_has_perf_metrics perhaps something like
> arch_evsel__pmu_has_topdown_events.

The topdown is a tricky feature. For the big core, to support the
topdown events, we have a dedicated perf metrics register, which has to
be grouped with the fixed counter 3. That brings all of these troubles.
Sorry for that.
For the atom, the topdown events are still supported, but with GP
counters. There is no perf metrics register at all.

The evsel__sys_has_perf_metrics() is used to check whether the perf
metrics register is supported. If so, we have to specially handle it.
It's not to check whether the topdown events are supported. So I think
it's better to keep the perf_metrics name, rather than topdown_events.

Thanks,
Kan
> 
> Thanks,
> Ian
> 
>>> +
>>>       return evsel->name &&
>>>               (strcasestr(evsel->name, "slots") ||
>>>                strcasestr(evsel->name, "topdown"));
Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs
Posted by Arnaldo Carvalho de Melo 3 years, 1 month ago 
Em Wed, Mar 01, 2023 at 08:12:06PM -0800, Ian Rogers escreveu:
> Don't just match on the event name, restict based on the PMU too.

restrict.

Can you please expand a bit this explanation?

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/evsel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index ea3972d785d1..580b0a172136 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>  	if (!evsel__sys_has_perf_metrics(evsel))
>  		return false;
>  
> +	if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
> +		return false;
> +
>  	return evsel->name &&
>  		(strcasestr(evsel->name, "slots") ||
>  		 strcasestr(evsel->name, "topdown"));
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.