diff mbox series

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

Message ID 20230302041211.852330-6-irogers@google.com
State New
Headers
Series Better fixes for grouping of events |

Commit Message

Ian Rogers March 2, 2023, 4:12 a.m. UTC 
  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(+)

Comments

Arnaldo Carvalho de Melo March 2, 2023, 2:35 p.m. UTC | #1 
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
>
Liang, Kan March 2, 2023, 3:24 p.m. UTC | #2 
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"));
Ian Rogers March 2, 2023, 7:38 p.m. UTC | #3 
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"));
Liang, Kan March 2, 2023, 8:28 p.m. UTC | #4 
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"));
diff mbox series

Patch

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