diff mbox series

[v5,28/34] perf pmus: Split pmus list into core and other

Message ID 20230527072210.2900565-29-irogers@google.com (mailing list archive)
State New, archived
Headers show
Series PMU refactoring and improvements | expand

Commit Message

Ian Rogers May 27, 2023, 7:22 a.m. UTC
Split the pmus list into core and other. This will later allow for
the core and other pmus to be populated separately.

Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 14 deletions(-)

Comments

Ravi Bangoria June 9, 2023, 3:59 a.m. UTC | #1
Hi Ian,

On 27-May-23 12:52 PM, Ian Rogers wrote:
> Split the pmus list into core and other. This will later allow for
> the core and other pmus to be populated separately.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 58ff7937e9b7..4ef4fecd335f 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -12,13 +12,19 @@
>  #include "pmu.h"
>  #include "print-events.h"
>  
> -static LIST_HEAD(pmus);
> +static LIST_HEAD(core_pmus);
> +static LIST_HEAD(other_pmus);

AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
So, is it safe to assume that other_pmus are not just uncore pmus? In that
case shall we add a comment here?

Thanks,
Ravi
Ian Rogers June 9, 2023, 4:40 a.m. UTC | #2
On Thu, Jun 8, 2023 at 9:01 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Ian,

Hi Ravi,

> On 27-May-23 12:52 PM, Ian Rogers wrote:
> > Split the pmus list into core and other. This will later allow for
> > the core and other pmus to be populated separately.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> > ---
> >  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 38 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index 58ff7937e9b7..4ef4fecd335f 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -12,13 +12,19 @@
> >  #include "pmu.h"
> >  #include "print-events.h"
> >
> > -static LIST_HEAD(pmus);
> > +static LIST_HEAD(core_pmus);
> > +static LIST_HEAD(other_pmus);
>
> AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
> core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
> So, is it safe to assume that other_pmus are not just uncore pmus? In that
> case shall we add a comment here?

I'm a fan of comments. The code has landed in perf-tools-next:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmus.c?h=perf-tools-next
Do you have any suggestions on wording? I've had limited success
adding glossary terms, for example, offcore vs uncore:
https://perf.wiki.kernel.org/index.php/Glossary#Offcore
I think offcore is a more interconnect related term, but I'd prefer
not to be inventing the definitions. I'd like it if we could be less
ambiguous in the code and provide useful information on the wiki, so
help appreciated :-)

Thanks,
Ian

> Thanks,
> Ravi
Ravi Bangoria June 9, 2023, 5:30 a.m. UTC | #3
On 09-Jun-23 10:10 AM, Ian Rogers wrote:
> On Thu, Jun 8, 2023 at 9:01 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> Hi Ian,
> 
> Hi Ravi,
> 
>> On 27-May-23 12:52 PM, Ian Rogers wrote:
>>> Split the pmus list into core and other. This will later allow for
>>> the core and other pmus to be populated separately.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
>>> ---
>>>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
>>>  1 file changed, 38 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
>>> index 58ff7937e9b7..4ef4fecd335f 100644
>>> --- a/tools/perf/util/pmus.c
>>> +++ b/tools/perf/util/pmus.c
>>> @@ -12,13 +12,19 @@
>>>  #include "pmu.h"
>>>  #include "print-events.h"
>>>
>>> -static LIST_HEAD(pmus);
>>> +static LIST_HEAD(core_pmus);
>>> +static LIST_HEAD(other_pmus);
>>
>> AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
>> core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
>> So, is it safe to assume that other_pmus are not just uncore pmus? In that
>> case shall we add a comment here?
> 
> I'm a fan of comments. The code has landed in perf-tools-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmus.c?h=perf-tools-next
> Do you have any suggestions on wording? I've had limited success
> adding glossary terms, for example, offcore vs uncore:
> https://perf.wiki.kernel.org/index.php/Glossary#Offcore
> I think offcore is a more interconnect related term, but I'd prefer
> not to be inventing the definitions. I'd like it if we could be less
> ambiguous in the code and provide useful information on the wiki, so
> help appreciated :-)

Does this look good?

/*
 * core_pmus:  A PMU belongs to core_pmus if it's name is "cpu" or it's sysfs
 *             directory contains "cpus" file. All PMUs belonging to core_pmus
 *             must have pmu->is_core=1. If there are more than one PMUs in
 *             this list, perf interprets it as a heterogeneous platform.
 * other_pmus: All other PMUs which are not part of core_pmus list. Does not
 *             matter whether it is a per SMT-thread or outside of the core in
 *             hw. i.e. PMUs belonging to other_pmus must have pmu->is_core=0
 *             but pmu->is_uncore could be 0 or 1.
 */
static LIST_HEAD(core_pmus);
static LIST_HEAD(other_pmus);

Thanks,
Ravi
Ian Rogers June 9, 2023, 5:35 a.m. UTC | #4
On Thu, Jun 8, 2023 at 10:30 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> On 09-Jun-23 10:10 AM, Ian Rogers wrote:
> > On Thu, Jun 8, 2023 at 9:01 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> >>
> >> Hi Ian,
> >
> > Hi Ravi,
> >
> >> On 27-May-23 12:52 PM, Ian Rogers wrote:
> >>> Split the pmus list into core and other. This will later allow for
> >>> the core and other pmus to be populated separately.
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> >>> ---
> >>>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
> >>>  1 file changed, 38 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> >>> index 58ff7937e9b7..4ef4fecd335f 100644
> >>> --- a/tools/perf/util/pmus.c
> >>> +++ b/tools/perf/util/pmus.c
> >>> @@ -12,13 +12,19 @@
> >>>  #include "pmu.h"
> >>>  #include "print-events.h"
> >>>
> >>> -static LIST_HEAD(pmus);
> >>> +static LIST_HEAD(core_pmus);
> >>> +static LIST_HEAD(other_pmus);
> >>
> >> AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
> >> core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
> >> So, is it safe to assume that other_pmus are not just uncore pmus? In that
> >> case shall we add a comment here?
> >
> > I'm a fan of comments. The code has landed in perf-tools-next:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmus.c?h=perf-tools-next
> > Do you have any suggestions on wording? I've had limited success
> > adding glossary terms, for example, offcore vs uncore:
> > https://perf.wiki.kernel.org/index.php/Glossary#Offcore
> > I think offcore is a more interconnect related term, but I'd prefer
> > not to be inventing the definitions. I'd like it if we could be less
> > ambiguous in the code and provide useful information on the wiki, so
> > help appreciated :-)
>
> Does this look good?
>
> /*
>  * core_pmus:  A PMU belongs to core_pmus if it's name is "cpu" or it's sysfs
>  *             directory contains "cpus" file. All PMUs belonging to core_pmus
>  *             must have pmu->is_core=1. If there are more than one PMUs in
>  *             this list, perf interprets it as a heterogeneous platform.


Looks good but a nit here. It is heterogeneous from point-of-view of
PMUs, there are ARM systems where they are heterogenous with big and
little cores but they have a single homogeneous PMU driver. The perf
tool will treat them as homogeneous.

Thanks,
Ian

>  * other_pmus: All other PMUs which are not part of core_pmus list. Does not
>  *             matter whether it is a per SMT-thread or outside of the core in
>  *             hw. i.e. PMUs belonging to other_pmus must have pmu->is_core=0
>  *             but pmu->is_uncore could be 0 or 1.
>  */
> static LIST_HEAD(core_pmus);
> static LIST_HEAD(other_pmus);
>
> Thanks,
> Ravi
Ravi Bangoria June 9, 2023, 5:55 a.m. UTC | #5
On 09-Jun-23 11:05 AM, Ian Rogers wrote:
> On Thu, Jun 8, 2023 at 10:30 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> On 09-Jun-23 10:10 AM, Ian Rogers wrote:
>>> On Thu, Jun 8, 2023 at 9:01 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>>>
>>>> Hi Ian,
>>>
>>> Hi Ravi,
>>>
>>>> On 27-May-23 12:52 PM, Ian Rogers wrote:
>>>>> Split the pmus list into core and other. This will later allow for
>>>>> the core and other pmus to be populated separately.
>>>>>
>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
>>>>> ---
>>>>>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
>>>>>  1 file changed, 38 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
>>>>> index 58ff7937e9b7..4ef4fecd335f 100644
>>>>> --- a/tools/perf/util/pmus.c
>>>>> +++ b/tools/perf/util/pmus.c
>>>>> @@ -12,13 +12,19 @@
>>>>>  #include "pmu.h"
>>>>>  #include "print-events.h"
>>>>>
>>>>> -static LIST_HEAD(pmus);
>>>>> +static LIST_HEAD(core_pmus);
>>>>> +static LIST_HEAD(other_pmus);
>>>>
>>>> AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
>>>> core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
>>>> So, is it safe to assume that other_pmus are not just uncore pmus? In that
>>>> case shall we add a comment here?
>>>
>>> I'm a fan of comments. The code has landed in perf-tools-next:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmus.c?h=perf-tools-next
>>> Do you have any suggestions on wording? I've had limited success
>>> adding glossary terms, for example, offcore vs uncore:
>>> https://perf.wiki.kernel.org/index.php/Glossary#Offcore
>>> I think offcore is a more interconnect related term, but I'd prefer
>>> not to be inventing the definitions. I'd like it if we could be less
>>> ambiguous in the code and provide useful information on the wiki, so
>>> help appreciated :-)
>>
>> Does this look good?
>>
>> /*
>>  * core_pmus:  A PMU belongs to core_pmus if it's name is "cpu" or it's sysfs
>>  *             directory contains "cpus" file. All PMUs belonging to core_pmus
>>  *             must have pmu->is_core=1. If there are more than one PMUs in
>>  *             this list, perf interprets it as a heterogeneous platform.
> 
> 
> Looks good but a nit here. It is heterogeneous from point-of-view of
> PMUs, there are ARM systems where they are heterogenous with big an> little cores but they have a single homogeneous PMU driver. The perf
> tool will treat them as homogeneous.

In that case number of entries in core_pmus list would still be 1 right?

Thanks,
Ravi
Ian Rogers June 9, 2023, 6 a.m. UTC | #6
On Thu, Jun 8, 2023 at 10:55 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> On 09-Jun-23 11:05 AM, Ian Rogers wrote:
> > On Thu, Jun 8, 2023 at 10:30 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> >>
> >> On 09-Jun-23 10:10 AM, Ian Rogers wrote:
> >>> On Thu, Jun 8, 2023 at 9:01 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> >>>>
> >>>> Hi Ian,
> >>>
> >>> Hi Ravi,
> >>>
> >>>> On 27-May-23 12:52 PM, Ian Rogers wrote:
> >>>>> Split the pmus list into core and other. This will later allow for
> >>>>> the core and other pmus to be populated separately.
> >>>>>
> >>>>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>>>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> >>>>> ---
> >>>>>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
> >>>>>  1 file changed, 38 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> >>>>> index 58ff7937e9b7..4ef4fecd335f 100644
> >>>>> --- a/tools/perf/util/pmus.c
> >>>>> +++ b/tools/perf/util/pmus.c
> >>>>> @@ -12,13 +12,19 @@
> >>>>>  #include "pmu.h"
> >>>>>  #include "print-events.h"
> >>>>>
> >>>>> -static LIST_HEAD(pmus);
> >>>>> +static LIST_HEAD(core_pmus);
> >>>>> +static LIST_HEAD(other_pmus);
> >>>>
> >>>> AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
> >>>> core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
> >>>> So, is it safe to assume that other_pmus are not just uncore pmus? In that
> >>>> case shall we add a comment here?
> >>>
> >>> I'm a fan of comments. The code has landed in perf-tools-next:
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmus.c?h=perf-tools-next
> >>> Do you have any suggestions on wording? I've had limited success
> >>> adding glossary terms, for example, offcore vs uncore:
> >>> https://perf.wiki.kernel.org/index.php/Glossary#Offcore
> >>> I think offcore is a more interconnect related term, but I'd prefer
> >>> not to be inventing the definitions. I'd like it if we could be less
> >>> ambiguous in the code and provide useful information on the wiki, so
> >>> help appreciated :-)
> >>
> >> Does this look good?
> >>
> >> /*
> >>  * core_pmus:  A PMU belongs to core_pmus if it's name is "cpu" or it's sysfs
> >>  *             directory contains "cpus" file. All PMUs belonging to core_pmus
> >>  *             must have pmu->is_core=1. If there are more than one PMUs in
> >>  *             this list, perf interprets it as a heterogeneous platform.
> >
> >
> > Looks good but a nit here. It is heterogeneous from point-of-view of
> > PMUs, there are ARM systems where they are heterogenous with big an> little cores but they have a single homogeneous PMU driver. The perf
> > tool will treat them as homogeneous.
>
> In that case number of entries in core_pmus list would still be 1 right?

Right. Heterogeneous platform, homogeneous PMU, single core PMU.

Thanks,
Ian

> Thanks,
> Ravi
Ravi Bangoria June 9, 2023, 6:02 a.m. UTC | #7
On 09-Jun-23 11:30 AM, Ian Rogers wrote:
> On Thu, Jun 8, 2023 at 10:55 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> On 09-Jun-23 11:05 AM, Ian Rogers wrote:
>>> On Thu, Jun 8, 2023 at 10:30 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>>>
>>>> On 09-Jun-23 10:10 AM, Ian Rogers wrote:
>>>>> On Thu, Jun 8, 2023 at 9:01 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>>>>>
>>>>>> Hi Ian,
>>>>>
>>>>> Hi Ravi,
>>>>>
>>>>>> On 27-May-23 12:52 PM, Ian Rogers wrote:
>>>>>>> Split the pmus list into core and other. This will later allow for
>>>>>>> the core and other pmus to be populated separately.
>>>>>>>
>>>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>>>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
>>>>>>> ---
>>>>>>>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
>>>>>>>  1 file changed, 38 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
>>>>>>> index 58ff7937e9b7..4ef4fecd335f 100644
>>>>>>> --- a/tools/perf/util/pmus.c
>>>>>>> +++ b/tools/perf/util/pmus.c
>>>>>>> @@ -12,13 +12,19 @@
>>>>>>>  #include "pmu.h"
>>>>>>>  #include "print-events.h"
>>>>>>>
>>>>>>> -static LIST_HEAD(pmus);
>>>>>>> +static LIST_HEAD(core_pmus);
>>>>>>> +static LIST_HEAD(other_pmus);
>>>>>>
>>>>>> AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
>>>>>> core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
>>>>>> So, is it safe to assume that other_pmus are not just uncore pmus? In that
>>>>>> case shall we add a comment here?
>>>>>
>>>>> I'm a fan of comments. The code has landed in perf-tools-next:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmus.c?h=perf-tools-next
>>>>> Do you have any suggestions on wording? I've had limited success
>>>>> adding glossary terms, for example, offcore vs uncore:
>>>>> https://perf.wiki.kernel.org/index.php/Glossary#Offcore
>>>>> I think offcore is a more interconnect related term, but I'd prefer
>>>>> not to be inventing the definitions. I'd like it if we could be less
>>>>> ambiguous in the code and provide useful information on the wiki, so
>>>>> help appreciated :-)
>>>>
>>>> Does this look good?
>>>>
>>>> /*
>>>>  * core_pmus:  A PMU belongs to core_pmus if it's name is "cpu" or it's sysfs
>>>>  *             directory contains "cpus" file. All PMUs belonging to core_pmus
>>>>  *             must have pmu->is_core=1. If there are more than one PMUs in
>>>>  *             this list, perf interprets it as a heterogeneous platform.
>>>
>>>
>>> Looks good but a nit here. It is heterogeneous from point-of-view of
>>> PMUs, there are ARM systems where they are heterogenous with big an> little cores but they have a single homogeneous PMU driver. The perf
>>> tool will treat them as homogeneous.
>>
>> In that case number of entries in core_pmus list would still be 1 right?
> 
> Right. Heterogeneous platform, homogeneous PMU, single core PMU.

Thanks for the clarification. I'll send a patch.

Ravi
Mark Rutland June 9, 2023, 7:58 a.m. UTC | #8
On Thu, Jun 08, 2023 at 10:35:02PM -0700, Ian Rogers wrote:
> On Thu, Jun 8, 2023 at 10:30 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> >
> > On 09-Jun-23 10:10 AM, Ian Rogers wrote:
> > > On Thu, Jun 8, 2023 at 9:01 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> > >>
> > >> Hi Ian,
> > >
> > > Hi Ravi,
> > >
> > >> On 27-May-23 12:52 PM, Ian Rogers wrote:
> > >>> Split the pmus list into core and other. This will later allow for
> > >>> the core and other pmus to be populated separately.
> > >>>
> > >>> Signed-off-by: Ian Rogers <irogers@google.com>
> > >>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> > >>> ---
> > >>>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
> > >>>  1 file changed, 38 insertions(+), 14 deletions(-)
> > >>>
> > >>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > >>> index 58ff7937e9b7..4ef4fecd335f 100644
> > >>> --- a/tools/perf/util/pmus.c
> > >>> +++ b/tools/perf/util/pmus.c
> > >>> @@ -12,13 +12,19 @@
> > >>>  #include "pmu.h"
> > >>>  #include "print-events.h"
> > >>>
> > >>> -static LIST_HEAD(pmus);
> > >>> +static LIST_HEAD(core_pmus);
> > >>> +static LIST_HEAD(other_pmus);
> > >>
> > >> AMD ibs_fetch// and ibs_op// PMUs are per SMT-thread and are independent of
> > >> core hw pmu. I wonder where does IBS fit. Currently it's part of other_pmus.
> > >> So, is it safe to assume that other_pmus are not just uncore pmus? In that
> > >> case shall we add a comment here?
> > >
> > > I'm a fan of comments. The code has landed in perf-tools-next:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmus.c?h=perf-tools-next
> > > Do you have any suggestions on wording? I've had limited success
> > > adding glossary terms, for example, offcore vs uncore:
> > > https://perf.wiki.kernel.org/index.php/Glossary#Offcore
> > > I think offcore is a more interconnect related term, but I'd prefer
> > > not to be inventing the definitions. I'd like it if we could be less
> > > ambiguous in the code and provide useful information on the wiki, so
> > > help appreciated :-)
> >
> > Does this look good?
> >
> > /*
> >  * core_pmus:  A PMU belongs to core_pmus if it's name is "cpu" or it's sysfs
> >  *             directory contains "cpus" file. All PMUs belonging to core_pmus
> >  *             must have pmu->is_core=1. If there are more than one PMUs in
> >  *             this list, perf interprets it as a heterogeneous platform.
> 
> 
> Looks good but a nit here. It is heterogeneous from point-of-view of
> PMUs, there are ARM systems where they are heterogenous with big and
> little cores but they have a single homogeneous PMU driver. The perf
> tool will treat them as homogeneous.

For the sake of the comment: there's a little more nuance here.

The intent is that each distinct micro-architecture has its own PMU instance,
but some people write their device trees incorrectly with a single pmu node
rather than separate pmu nodes per micro-architecture.

That should be viewed as a FW bug, even if we have to deal with it here.

Thanks,
Mark.
diff mbox series

Patch

diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 58ff7937e9b7..4ef4fecd335f 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -12,13 +12,19 @@ 
 #include "pmu.h"
 #include "print-events.h"
 
-static LIST_HEAD(pmus);
+static LIST_HEAD(core_pmus);
+static LIST_HEAD(other_pmus);
 
 void perf_pmus__destroy(void)
 {
 	struct perf_pmu *pmu, *tmp;
 
-	list_for_each_entry_safe(pmu, tmp, &pmus, list) {
+	list_for_each_entry_safe(pmu, tmp, &core_pmus, list) {
+		list_del(&pmu->list);
+
+		perf_pmu__delete(pmu);
+	}
+	list_for_each_entry_safe(pmu, tmp, &other_pmus, list) {
 		list_del(&pmu->list);
 
 		perf_pmu__delete(pmu);
@@ -29,7 +35,12 @@  static struct perf_pmu *pmu_find(const char *name)
 {
 	struct perf_pmu *pmu;
 
-	list_for_each_entry(pmu, &pmus, list) {
+	list_for_each_entry(pmu, &core_pmus, list) {
+		if (!strcmp(pmu->name, name) ||
+		    (pmu->alias_name && !strcmp(pmu->alias_name, name)))
+			return pmu;
+	}
+	list_for_each_entry(pmu, &other_pmus, list) {
 		if (!strcmp(pmu->name, name) ||
 		    (pmu->alias_name && !strcmp(pmu->alias_name, name)))
 			return pmu;
@@ -53,7 +64,7 @@  struct perf_pmu *perf_pmus__find(const char *name)
 		return pmu;
 
 	dirfd = perf_pmu__event_source_devices_fd();
-	pmu = perf_pmu__lookup(&pmus, dirfd, name);
+	pmu = perf_pmu__lookup(is_pmu_core(name) ? &core_pmus : &other_pmus, dirfd, name);
 	close(dirfd);
 
 	return pmu;
@@ -72,7 +83,7 @@  static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
 	if (pmu)
 		return pmu;
 
-	return perf_pmu__lookup(&pmus, dirfd, name);
+	return perf_pmu__lookup(is_pmu_core(name) ? &core_pmus : &other_pmus, dirfd, name);
 }
 
 /* Add all pmus in sysfs to pmu list: */
@@ -93,7 +104,7 @@  static void pmu_read_sysfs(void)
 	while ((dent = readdir(dir))) {
 		if (!strcmp(dent->d_name, ".") || !strcmp(dent->d_name, ".."))
 			continue;
-		/* add to static LIST_HEAD(pmus): */
+		/* add to static LIST_HEAD(core_pmus) or LIST_HEAD(other_pmus): */
 		perf_pmu__find2(fd, dent->d_name);
 	}
 
@@ -104,24 +115,37 @@  struct perf_pmu *perf_pmus__find_by_type(unsigned int type)
 {
 	struct perf_pmu *pmu;
 
-	list_for_each_entry(pmu, &pmus, list)
+	list_for_each_entry(pmu, &core_pmus, list) {
 		if (pmu->type == type)
 			return pmu;
-
+	}
+	list_for_each_entry(pmu, &other_pmus, list) {
+		if (pmu->type == type)
+			return pmu;
+	}
 	return NULL;
 }
 
+/*
+ * pmu iterator: If pmu is NULL, we start at the begin, otherwise return the
+ * next pmu. Returns NULL on end.
+ */
 struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu)
 {
-	/*
-	 * pmu iterator: If pmu is NULL, we start at the begin,
-	 * otherwise return the next pmu. Returns NULL on end.
-	 */
+	bool use_core_pmus = !pmu || pmu->is_core;
+
 	if (!pmu) {
 		pmu_read_sysfs();
-		pmu = list_prepare_entry(pmu, &pmus, list);
+		pmu = list_prepare_entry(pmu, &core_pmus, list);
+	}
+	if (use_core_pmus) {
+		list_for_each_entry_continue(pmu, &core_pmus, list)
+			return pmu;
+
+		pmu = NULL;
+		pmu = list_prepare_entry(pmu, &other_pmus, list);
 	}
-	list_for_each_entry_continue(pmu, &pmus, list)
+	list_for_each_entry_continue(pmu, &other_pmus, list)
 		return pmu;
 	return NULL;
 }