diff mbox series

[RFC,2/5] perf jevents: add support for arch recommended events

Message ID 1512490399-94107-3-git-send-email-john.garry@huawei.com
State New
Headers show
Series perf events patches for improved ARM64 support | expand

Commit Message

John Garry Dec. 5, 2017, 4:13 p.m. UTC
For some architectures (like arm64), there are architecture-
defined recommended events. Vendors may not be obliged to
follow the recommendation and may implement their own pmu
event for a specific event code.

This patch adds support for parsing events from arch-defined
recommended JSONs, and then fixing up vendor events when
they have implemented these events as recommended.

In the vendor JSON, to specify that the event is supported
according to the recommendation, only the event code is
added to the JSON entry - no other event elements need be
added, like below:
[
    {
        "EventCode": "0x40",
    },

]

The pmu event parsing will check for "BriefDescription"
field presence only for this.

If "BriefDescription" is present, then it is implied
that the vendor has implemented their own custom event,
and there is no fixup. Other fields are ignored.

*TODO: update documentation

Signed-off-by: John Garry <john.garry@huawei.com>

---
 tools/perf/pmu-events/jevents.c | 215 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 198 insertions(+), 17 deletions(-)

-- 
1.9.1

Comments

Andi Kleen Dec. 5, 2017, 5:27 p.m. UTC | #1
On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:
> For some architectures (like arm64), there are architecture-

> defined recommended events. Vendors may not be obliged to

> follow the recommendation and may implement their own pmu

> event for a specific event cod


I would just duplicate the architected events into the different 
vendor files.  Then you wouldn't need all this mess.

-Andi
John Garry Dec. 6, 2017, 8:34 a.m. UTC | #2
On 05/12/2017 17:27, Andi Kleen wrote:
> On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

>> For some architectures (like arm64), there are architecture-

>> defined recommended events. Vendors may not be obliged to

>> follow the recommendation and may implement their own pmu

>> event for a specific event cod

>

> I would just duplicate the architected events into the different

> vendor files.  Then you wouldn't need all this mess.

>


This is what we were originally doing:
https://patchwork.kernel.org/patch/10010859/

But then we thought that we could avoid duplicating all these events for 
every platform from every vendor. Most, if not all, vendors will 
implement the events as recommended for any platform, so much 
unnecessary duplication.

cheers,
John


> -Andi

>

> _______________________________________________

> linux-arm-kernel mailing list

> linux-arm-kernel@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

>

> .

>
Jiri Olsa Dec. 6, 2017, 1:36 p.m. UTC | #3
On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:
> For some architectures (like arm64), there are architecture-

> defined recommended events. Vendors may not be obliged to

> follow the recommendation and may implement their own pmu

> event for a specific event code.

> 

> This patch adds support for parsing events from arch-defined

> recommended JSONs, and then fixing up vendor events when

> they have implemented these events as recommended.


in the previous patch you added the vendor support, so
you have arch|vendor|platform key for the event list
and perf have the most current/local event list

why would you need to fix it? if there's new event list,
the table gets updated, perf is rebuilt.. I'm clearly
missing something ;-)

> In the vendor JSON, to specify that the event is supported

> according to the recommendation, only the event code is

> added to the JSON entry - no other event elements need be

> added, like below:

> [

>     {

>         "EventCode": "0x40",

>     },

> 

> ]

> 

> The pmu event parsing will check for "BriefDescription"

> field presence only for this.

> 

> If "BriefDescription" is present, then it is implied

> that the vendor has implemented their own custom event,

> and there is no fixup. Other fields are ignored.


if we are going this way, please use some new token,
this list is supposed to be human readable

thanks,
jirka
Jiri Olsa Dec. 6, 2017, 1:37 p.m. UTC | #4
On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

SNIP

> ---

>  tools/perf/pmu-events/jevents.c | 215 ++++++++++++++++++++++++++++++++++++----

>  1 file changed, 198 insertions(+), 17 deletions(-)

> 

> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c

> index a0d489e..a820ed4 100644

> --- a/tools/perf/pmu-events/jevents.c

> +++ b/tools/perf/pmu-events/jevents.c

> @@ -42,6 +42,7 @@

>  #include <dirent.h>

>  #include <sys/time.h>			/* getrlimit */

>  #include <sys/resource.h>		/* getrlimit */

> +#include <sys/queue.h>

>  #include <ftw.h>

>  #include <sys/stat.h>

>  #include "jsmn.h"

> @@ -366,6 +367,94 @@ static int print_events_table_entry(void *data, char *name, char *event,

>  	return 0;

>  }

>  

> +struct event_struct {

> +	char *name;

> +	char *event;

> +	char *desc;

> +	char *long_desc;

> +	char *pmu;

> +	char *unit;

> +	char *perpkg;

> +	char *metric_expr;

> +	char *metric_name;

> +	char *metric_group;

> +	LIST_ENTRY(event_struct) list;


is there any reason you don't use the 'struct list_head' ?
I dont think we want another thingie involved for lists

jirka
John Garry Dec. 6, 2017, 2:40 p.m. UTC | #5
On 06/12/2017 13:37, Jiri Olsa wrote:
> On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

>

> SNIP

>

>> ---

>>  tools/perf/pmu-events/jevents.c | 215 ++++++++++++++++++++++++++++++++++++----

>>  1 file changed, 198 insertions(+), 17 deletions(-)

>>

>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c

>> index a0d489e..a820ed4 100644

>> --- a/tools/perf/pmu-events/jevents.c

>> +++ b/tools/perf/pmu-events/jevents.c

>> @@ -42,6 +42,7 @@

>>  #include <dirent.h>

>>  #include <sys/time.h>			/* getrlimit */

>>  #include <sys/resource.h>		/* getrlimit */

>> +#include <sys/queue.h>

>>  #include <ftw.h>

>>  #include <sys/stat.h>

>>  #include "jsmn.h"

>> @@ -366,6 +367,94 @@ static int print_events_table_entry(void *data, char *name, char *event,

>>  	return 0;

>>  }

>>

>> +struct event_struct {

>> +	char *name;

>> +	char *event;

>> +	char *desc;

>> +	char *long_desc;

>> +	char *pmu;

>> +	char *unit;

>> +	char *perpkg;

>> +	char *metric_expr;

>> +	char *metric_name;

>> +	char *metric_group;

>> +	LIST_ENTRY(event_struct) list;

>

> is there any reason you don't use the 'struct list_head' ?

> I dont think we want another thingie involved for lists


Hi jirka,

The linux kernel headers are not used for jevents tool. I would rather 
use them if possible...

Thanks,
John

>

> jirka

>

> .

>
John Garry Dec. 6, 2017, 3:20 p.m. UTC | #6
On 06/12/2017 13:36, Jiri Olsa wrote:
> On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

>> For some architectures (like arm64), there are architecture-

>> defined recommended events. Vendors may not be obliged to

>> follow the recommendation and may implement their own pmu

>> event for a specific event code.

>>

>> This patch adds support for parsing events from arch-defined

>> recommended JSONs, and then fixing up vendor events when

>> they have implemented these events as recommended.

>

> in the previous patch you added the vendor support, so

> you have arch|vendor|platform key for the event list

> and perf have the most current/local event list

>

> why would you need to fix it? if there's new event list,

> the table gets updated, perf is rebuilt.. I'm clearly

> missing something ;-)


The 2 patches are quite separate. In the first patch, I just added 
support for the vendor subdirectory.

So this patch is not related to rebuilding when adding a new event list 
or dependency checking.

Here we are trying to allow the vendor to just specify that an event is 
supported as standard in their platform, without duplicating all the 
standard event fields in their JSON. When processing the vendor JSONs, 
the jevents tool can figure which events are standard and create the 
proper event entries in the pmu events table, referencing the 
architecture JSON.

>

>> In the vendor JSON, to specify that the event is supported

>> according to the recommendation, only the event code is

>> added to the JSON entry - no other event elements need be

>> added, like below:

>> [

>>     {

>>         "EventCode": "0x40",

>>     },

>>

>> ]

>>

>> The pmu event parsing will check for "BriefDescription"

>> field presence only for this.

>>

>> If "BriefDescription" is present, then it is implied

>> that the vendor has implemented their own custom event,

>> and there is no fixup. Other fields are ignored.

>

> if we are going this way, please use some new token,

> this list is supposed to be human readable


A new token could work also, but it would be just a flag to mark the 
event "standard".

Ideally we could reference another entry in another JSON, like a 
pointer, but I don't think that this is possible with JSONs; not unless 
we introduce some elaborate custom scheme to allow JSONs to be 
cross-referenced.

Cheers,
John

>

> thanks,

> jirka

>

> .

>
Jiri Olsa Dec. 8, 2017, 12:29 p.m. UTC | #7
On Wed, Dec 06, 2017 at 03:20:14PM +0000, John Garry wrote:
> On 06/12/2017 13:36, Jiri Olsa wrote:

> > On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

> > > For some architectures (like arm64), there are architecture-

> > > defined recommended events. Vendors may not be obliged to

> > > follow the recommendation and may implement their own pmu

> > > event for a specific event code.

> > > 

> > > This patch adds support for parsing events from arch-defined

> > > recommended JSONs, and then fixing up vendor events when

> > > they have implemented these events as recommended.

> > 

> > in the previous patch you added the vendor support, so

> > you have arch|vendor|platform key for the event list

> > and perf have the most current/local event list

> > 

> > why would you need to fix it? if there's new event list,

> > the table gets updated, perf is rebuilt.. I'm clearly

> > missing something ;-)

> 

> The 2 patches are quite separate. In the first patch, I just added support

> for the vendor subdirectory.

> 

> So this patch is not related to rebuilding when adding a new event list or

> dependency checking.

> 

> Here we are trying to allow the vendor to just specify that an event is

> supported as standard in their platform, without duplicating all the

> standard event fields in their JSON. When processing the vendor JSONs, the

> jevents tool can figure which events are standard and create the proper

> event entries in the pmu events table, referencing the architecture JSON.


I think we should keep this simple and mangle this with some pointer logic

now you have arch/vendor/platform directory structure.. why don't
you add events for every such directory? I understand there will
be duplications, but we already have them for other archs and it's
not big deal:

	[jolsa@krava perf]$ grep -r L2_RQSTS.DEMAND_DATA_RD_MISS pmu-events/arch/*
	pmu-events/arch/x86/broadwell/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",
	pmu-events/arch/x86/haswell/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",
	pmu-events/arch/x86/broadwellde/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",
	pmu-events/arch/x86/haswellx/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",
	pmu-events/arch/x86/skylake/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",
	pmu-events/arch/x86/skylakex/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",
	pmu-events/arch/x86/broadwellx/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

thanks,
jirka
Jiri Olsa Dec. 8, 2017, 12:31 p.m. UTC | #8
On Wed, Dec 06, 2017 at 02:40:10PM +0000, John Garry wrote:
> On 06/12/2017 13:37, Jiri Olsa wrote:

> > On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

> > 

> > SNIP

> > 

> > > ---

> > >  tools/perf/pmu-events/jevents.c | 215 ++++++++++++++++++++++++++++++++++++----

> > >  1 file changed, 198 insertions(+), 17 deletions(-)

> > > 

> > > diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c

> > > index a0d489e..a820ed4 100644

> > > --- a/tools/perf/pmu-events/jevents.c

> > > +++ b/tools/perf/pmu-events/jevents.c

> > > @@ -42,6 +42,7 @@

> > >  #include <dirent.h>

> > >  #include <sys/time.h>			/* getrlimit */

> > >  #include <sys/resource.h>		/* getrlimit */

> > > +#include <sys/queue.h>

> > >  #include <ftw.h>

> > >  #include <sys/stat.h>

> > >  #include "jsmn.h"

> > > @@ -366,6 +367,94 @@ static int print_events_table_entry(void *data, char *name, char *event,

> > >  	return 0;

> > >  }

> > > 

> > > +struct event_struct {

> > > +	char *name;

> > > +	char *event;

> > > +	char *desc;

> > > +	char *long_desc;

> > > +	char *pmu;

> > > +	char *unit;

> > > +	char *perpkg;

> > > +	char *metric_expr;

> > > +	char *metric_name;

> > > +	char *metric_group;

> > > +	LIST_ENTRY(event_struct) list;

> > 

> > is there any reason you don't use the 'struct list_head' ?

> > I dont think we want another thingie involved for lists

> 

> Hi jirka,

> 

> The linux kernel headers are not used for jevents tool. I would rather use

> them if possible...


should be as easy as adding  #include <linux/list.h> ;-)

it's heavily used within perf and I'm pretty sure we want
to keep around just one way of doing lists

thanks,
jirka
John Garry Dec. 8, 2017, 3:38 p.m. UTC | #9
On 08/12/2017 12:31, Jiri Olsa wrote:
> On Wed, Dec 06, 2017 at 02:40:10PM +0000, John Garry wrote:

>> On 06/12/2017 13:37, Jiri Olsa wrote:

>>> On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

>>>

>>> SNIP

>>>

>>>> ---

>>>>  tools/perf/pmu-events/jevents.c | 215 ++++++++++++++++++++++++++++++++++++----

>>>>  1 file changed, 198 insertions(+), 17 deletions(-)

>>>>

>>>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c

>>>> index a0d489e..a820ed4 100644

>>>> --- a/tools/perf/pmu-events/jevents.c

>>>> +++ b/tools/perf/pmu-events/jevents.c

>>>> @@ -42,6 +42,7 @@

>>>>  #include <dirent.h>

>>>>  #include <sys/time.h>			/* getrlimit */

>>>>  #include <sys/resource.h>		/* getrlimit */

>>>> +#include <sys/queue.h>

>>>>  #include <ftw.h>

>>>>  #include <sys/stat.h>

>>>>  #include "jsmn.h"

>>>> @@ -366,6 +367,94 @@ static int print_events_table_entry(void *data, char *name, char *event,

>>>>  	return 0;

>>>>  }

>>>>

>>>> +struct event_struct {

>>>> +	char *name;

>>>> +	char *event;

>>>> +	char *desc;

>>>> +	char *long_desc;

>>>> +	char *pmu;

>>>> +	char *unit;

>>>> +	char *perpkg;

>>>> +	char *metric_expr;

>>>> +	char *metric_name;

>>>> +	char *metric_group;

>>>> +	LIST_ENTRY(event_struct) list;

>>>

>>> is there any reason you don't use the 'struct list_head' ?

>>> I dont think we want another thingie involved for lists

>>

>> Hi jirka,

>>


Hi jirka,

>> The linux kernel headers are not used for jevents tool. I would rather use

>> them if possible...

>

> should be as easy as adding  #include <linux/list.h> ;-)

>


Hi jirka,

I think the issue is that jevents is a "hostprogs", which does not use 
kernel headers.

FWIW, here is the symptom:
pmu-events/jevents.c:51:24: fatal error: linux/list.h: No such file or 
directory
  #include <linux/list.h>
                         ^
compilation terminated.
mv: cannot stat ‘pmu-events/.jevents.o.tmp’: No such file or directory

perf tool build is different.

Much appreciated,
John


> it's heavily used within perf and I'm pretty sure we want

> to keep around just one way of doing lists

>

> thanks,

> jirka

>

> .

>
John Garry Dec. 8, 2017, 3:42 p.m. UTC | #10
On 08/12/2017 12:29, Jiri Olsa wrote:
> On Wed, Dec 06, 2017 at 03:20:14PM +0000, John Garry wrote:

>> On 06/12/2017 13:36, Jiri Olsa wrote:

>>> On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

>>>> For some architectures (like arm64), there are architecture-

>>>> defined recommended events. Vendors may not be obliged to

>>>> follow the recommendation and may implement their own pmu

>>>> event for a specific event code.

>>>>

>>>> This patch adds support for parsing events from arch-defined

>>>> recommended JSONs, and then fixing up vendor events when

>>>> they have implemented these events as recommended.

>>>

>>> in the previous patch you added the vendor support, so

>>> you have arch|vendor|platform key for the event list

>>> and perf have the most current/local event list

>>>

>>> why would you need to fix it? if there's new event list,

>>> the table gets updated, perf is rebuilt.. I'm clearly

>>> missing something ;-)

>>

>> The 2 patches are quite separate. In the first patch, I just added support

>> for the vendor subdirectory.

>>

>> So this patch is not related to rebuilding when adding a new event list or

>> dependency checking.

>>

>> Here we are trying to allow the vendor to just specify that an event is

>> supported as standard in their platform, without duplicating all the

>> standard event fields in their JSON. When processing the vendor JSONs, the

>> jevents tool can figure which events are standard and create the proper

>> event entries in the pmu events table, referencing the architecture JSON.

>


Hi jirka,

> I think we should keep this simple and mangle this with some pointer logic

>

> now you have arch/vendor/platform directory structure..


I'm glad that there seems to be no objection to this, as I feel that 
this was a problem.

why don't
> you add events for every such directory? I understand there will

> be duplications, but we already have them for other archs and it's

> not big deal:


The amount of duplication was the concern. As mentioned earlier, it 
would be anticipated that every vendor would implement these events as 
recommended, so a copy for every platform from every vendor. We're 
looking for a way to avoid this.

Actually having a scalable JSON standard format for pmu events, which 
allows us to define common events per architecture / vendor and 
reference them per platform JSON could be useful.

Here we're dealing with trade-off between duplication (simplicity) vs 
complexity (or over-engineering).

>

> 	[jolsa@krava perf]$ grep -r L2_RQSTS.DEMAND_DATA_RD_MISS pmu-events/arch/*

> 	pmu-events/arch/x86/broadwell/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

> 	pmu-events/arch/x86/haswell/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

> 	pmu-events/arch/x86/broadwellde/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

> 	pmu-events/arch/x86/haswellx/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

> 	pmu-events/arch/x86/skylake/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

> 	pmu-events/arch/x86/skylakex/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

> 	pmu-events/arch/x86/broadwellx/cache.json:        "EventName": "L2_RQSTS.DEMAND_DATA_RD_MISS",

>

> thanks,

> jirka


Cheers,
John

>

> .

>
Jiri Olsa Dec. 9, 2017, 7:26 a.m. UTC | #11
On Fri, Dec 08, 2017 at 03:38:06PM +0000, John Garry wrote:

SNIP

> > > 

> > > Hi jirka,

> > > 

> 

> Hi jirka,

> 

> > > The linux kernel headers are not used for jevents tool. I would rather use

> > > them if possible...

> > 

> > should be as easy as adding  #include <linux/list.h> ;-)

> > 

> 

> Hi jirka,

> 

> I think the issue is that jevents is a "hostprogs", which does not use

> kernel headers.

> 

> FWIW, here is the symptom:

> pmu-events/jevents.c:51:24: fatal error: linux/list.h: No such file or

> directory

>  #include <linux/list.h>

>                         ^

> compilation terminated.

> mv: cannot stat ‘pmu-events/.jevents.o.tmp’: No such file or directory

> 

> perf tool build is different.


yep, need additional in Bukld file, attached

jirka


---
diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build
index 999a4e878162..b7d2e0e9cbd0 100644
--- a/tools/perf/pmu-events/Build
+++ b/tools/perf/pmu-events/Build
@@ -1,5 +1,6 @@
 hostprogs := jevents
 
+CHOSTFLAGS      = -I$(srctree)/tools/include
 jevents-y	+= json.o jsmn.o jevents.o
 pmu-events-y	+= pmu-events.o
 JDIR		=  pmu-events/arch/$(SRCARCH)
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index b578aa26e375..5b9b1fee3dfe 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -47,6 +47,7 @@
 #include "jsmn.h"
 #include "json.h"
 #include "jevents.h"
+#include <linux/list.h>
 
 int verbose;
 char *prog;
@@ -884,6 +885,7 @@ int main(int argc, char *argv[])
 	const char *output_file;
 	const char *start_dirname;
 	struct stat stbuf;
+	struct list_head krava __maybe_unused;
 
 	prog = basename(argv[0]);
 	if (argc < 4) {
Jiri Olsa Dec. 9, 2017, 7:31 a.m. UTC | #12
On Fri, Dec 08, 2017 at 03:42:10PM +0000, John Garry wrote:
> On 08/12/2017 12:29, Jiri Olsa wrote:

> > On Wed, Dec 06, 2017 at 03:20:14PM +0000, John Garry wrote:

> > > On 06/12/2017 13:36, Jiri Olsa wrote:

> > > > On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

> > > > > For some architectures (like arm64), there are architecture-

> > > > > defined recommended events. Vendors may not be obliged to

> > > > > follow the recommendation and may implement their own pmu

> > > > > event for a specific event code.

> > > > > 

> > > > > This patch adds support for parsing events from arch-defined

> > > > > recommended JSONs, and then fixing up vendor events when

> > > > > they have implemented these events as recommended.

> > > > 

> > > > in the previous patch you added the vendor support, so

> > > > you have arch|vendor|platform key for the event list

> > > > and perf have the most current/local event list

> > > > 

> > > > why would you need to fix it? if there's new event list,

> > > > the table gets updated, perf is rebuilt.. I'm clearly

> > > > missing something ;-)

> > > 

> > > The 2 patches are quite separate. In the first patch, I just added support

> > > for the vendor subdirectory.

> > > 

> > > So this patch is not related to rebuilding when adding a new event list or

> > > dependency checking.

> > > 

> > > Here we are trying to allow the vendor to just specify that an event is

> > > supported as standard in their platform, without duplicating all the

> > > standard event fields in their JSON. When processing the vendor JSONs, the

> > > jevents tool can figure which events are standard and create the proper

> > > event entries in the pmu events table, referencing the architecture JSON.

> > 

> 

> Hi jirka,

> 

> > I think we should keep this simple and mangle this with some pointer logic


sry for confusion, of course it should have been '.. and NOT mangle..' ;-)

> > 

> > now you have arch/vendor/platform directory structure..

> 

> I'm glad that there seems to be no objection to this, as I feel that this

> was a problem.

> 

> why don't

> > you add events for every such directory? I understand there will

> > be duplications, but we already have them for other archs and it's

> > not big deal:

> 

> The amount of duplication was the concern. As mentioned earlier, it would be

> anticipated that every vendor would implement these events as recommended,

> so a copy for every platform from every vendor. We're looking for a way to

> avoid this.

> 

> Actually having a scalable JSON standard format for pmu events, which allows

> us to define common events per architecture / vendor and reference them per

> platform JSON could be useful.

> 

> Here we're dealing with trade-off between duplication (simplicity) vs

> complexity (or over-engineering).


understood, but as I said we already are ok with duplicates,
if it's reasonable size as is for x86 now..  how much amount
are we talking about for arm?

jirka
John Garry Dec. 11, 2017, 10:25 a.m. UTC | #13
On 09/12/2017 07:31, Jiri Olsa wrote:
> On Fri, Dec 08, 2017 at 03:42:10PM +0000, John Garry wrote:

>> On 08/12/2017 12:29, Jiri Olsa wrote:

>>> On Wed, Dec 06, 2017 at 03:20:14PM +0000, John Garry wrote:

>>>> On 06/12/2017 13:36, Jiri Olsa wrote:

>>>>> On Wed, Dec 06, 2017 at 12:13:16AM +0800, John Garry wrote:

>>>>>> For some architectures (like arm64), there are architecture-

>>>>>> defined recommended events. Vendors may not be obliged to

>>>>>> follow the recommendation and may implement their own pmu

>>>>>> event for a specific event code.

>>>>>>

>>>>>> This patch adds support for parsing events from arch-defined

>>>>>> recommended JSONs, and then fixing up vendor events when

>>>>>> they have implemented these events as recommended.

>>>>>

>>>>> in the previous patch you added the vendor support, so

>>>>> you have arch|vendor|platform key for the event list

>>>>> and perf have the most current/local event list

>>>>>

>>>>> why would you need to fix it? if there's new event list,

>>>>> the table gets updated, perf is rebuilt.. I'm clearly

>>>>> missing something ;-)

>>>>

>>>> The 2 patches are quite separate. In the first patch, I just added support

>>>> for the vendor subdirectory.

>>>>

>>>> So this patch is not related to rebuilding when adding a new event list or

>>>> dependency checking.

>>>>

>>>> Here we are trying to allow the vendor to just specify that an event is

>>>> supported as standard in their platform, without duplicating all the

>>>> standard event fields in their JSON. When processing the vendor JSONs, the

>>>> jevents tool can figure which events are standard and create the proper

>>>> event entries in the pmu events table, referencing the architecture JSON.

>>>

>>

>> Hi jirka,

>>

>>> I think we should keep this simple and mangle this with some pointer logic

>

> sry for confusion, of course it should have been '.. and NOT mangle..' ;-)

>

>>>

>>> now you have arch/vendor/platform directory structure..

>>

>> I'm glad that there seems to be no objection to this, as I feel that this

>> was a problem.

>>

>> why don't

>>> you add events for every such directory? I understand there will

>>> be duplications, but we already have them for other archs and it's

>>> not big deal:

>>

>> The amount of duplication was the concern. As mentioned earlier, it would be

>> anticipated that every vendor would implement these events as recommended,

>> so a copy for every platform from every vendor. We're looking for a way to

>> avoid this.

>>

>> Actually having a scalable JSON standard format for pmu events, which allows

>> us to define common events per architecture / vendor and reference them per

>> platform JSON could be useful.

>>

>> Here we're dealing with trade-off between duplication (simplicity) vs

>> complexity (or over-engineering).

>

> understood, but as I said we already are ok with duplicates,

> if it's reasonable size as is for x86 now..  how much amount

> are we talking about for arm?

>


Hi jirka,

These JSONs would only apply to vendors which have custom ARMv8 
implementations. If you check the ARMv8 ARM, there's 10 such companies 
recorded as ARMv8 implementators.

So this means that in the future we could have tens to hundreds of JSONs 
for arm64, all with these duplicated events.

At this point I'll ask Will Deacon to share his thoughts, as he 
originally requested this feature.

Thanks,
John

> jirka

>

> .

>
John Garry Dec. 15, 2017, 11:22 a.m. UTC | #14
>> Actually having a scalable JSON standard format for pmu events, which allows

>> us to define common events per architecture / vendor and reference them per

>> platform JSON could be useful.

>>

>> Here we're dealing with trade-off between duplication (simplicity) vs

>> complexity (or over-engineering).

>

> understood, but as I said we already are ok with duplicates,

> if it's reasonable size as is for x86 now..  how much amount

> are we talking about for arm?

>


Hi Jirka,

When you say reasonable size for x86, I ran a string duplication finder 
on the x86 JSONs and the results show a huge amount of duplication. 
Please check this:
https://gist.githubusercontent.com/johnpgarry/68bc87e823ae2ce0f7b475b4e55e5795/raw/f4cea138999d8b34151b9586d733592e01774d7a/x86%2520JSON%2520duplication

Extract:
"Found a 65 line (311 tokens) duplication in the following files:
Starting at line 100 of 
/linux/tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json
Starting at line 100 of 
/linux/tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json
Starting at line 100 of 
/linux/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
Starting at line 100 of 
/linux/tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json
Starting at line 76 of 
/linux/tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json
Starting at line 100 of 
/linux/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json
Starting at line 76 of 
/linux/tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json
Starting at line 100 of 
/linux/tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json"

Won't this all potentially have a big maintainence cost?

For example, I saw multiple JSON update patches which look identical:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/tools/perf/pmu-events/arch/x86?h=v4.15-rc3&id=7347bba5552f479d4292ffd008d18d41a965f021

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/tools/perf/pmu-events/arch/x86?h=v4.15-rc3&id=984d91f4c62f64026cbfef51f609971025934cec

I just don't know how this schema scales with more archs and more 
platforms supported. It's just early days now...

Regards,
John

> jirka

>

> .

>
Andi Kleen Dec. 16, 2017, 6:47 p.m. UTC | #15
> Won't this all potentially have a big maintainence cost?


No. It's all auto generated.

The only cost is slightly bigger binary size.

I would hope your event files are auto generated too.

> I just don't know how this schema scales with more archs and more platforms

> supported. It's just early days now...


Only perf will get slightly bigger, but memory is not exactly expensive.

In fact, the extra memory won't even be faulted in if it's not used,
so it's only disk space.

-Andi
Jiri Olsa Dec. 21, 2017, 7:39 p.m. UTC | #16
On Fri, Dec 15, 2017 at 11:22:50AM +0000, John Garry wrote:
> > > Actually having a scalable JSON standard format for pmu events, which allows

> > > us to define common events per architecture / vendor and reference them per

> > > platform JSON could be useful.

> > > 

> > > Here we're dealing with trade-off between duplication (simplicity) vs

> > > complexity (or over-engineering).

> > 

> > understood, but as I said we already are ok with duplicates,

> > if it's reasonable size as is for x86 now..  how much amount

> > are we talking about for arm?

> > 

> 

> Hi Jirka,

> 

> When you say reasonable size for x86, I ran a string duplication finder on

> the x86 JSONs and the results show a huge amount of duplication. Please

> check this:

> https://gist.githubusercontent.com/johnpgarry/68bc87e823ae2ce0f7b475b4e55e5795/raw/f4cea138999d8b34151b9586d733592e01774d7a/x86%2520JSON%2520duplication

> 

> Extract:

> "Found a 65 line (311 tokens) duplication in the following files:

> Starting at line 100 of

> /linux/tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json

> Starting at line 100 of

> /linux/tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json

> Starting at line 100 of

> /linux/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json

> Starting at line 100 of

> /linux/tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json

> Starting at line 76 of

> /linux/tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json

> Starting at line 100 of

> /linux/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json

> Starting at line 76 of

> /linux/tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json

> Starting at line 100 of

> /linux/tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json"

> 

> Won't this all potentially have a big maintainence cost?


as Andi said it's mostly just the disk space,
which is not big deal

I'm not doing JSON file updates, but I think having
simple single dir for platform/cpu could save us some
confusion in future

however I won't oppose if you want to add this logic,
but please:
  - use the list_head ;-)
  - leave the process_one_file function simple
    and separate the level0 processing
  - you are using 'EventCode' as an unique ID to find
    the base, but it's not unique for x86, you'll need
    to add some other ID scheme that fits to all archs

thanks,
jirka

> 

> For example, I saw multiple JSON update patches which look identical:

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/tools/perf/pmu-events/arch/x86?h=v4.15-rc3&id=7347bba5552f479d4292ffd008d18d41a965f021

> 

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/tools/perf/pmu-events/arch/x86?h=v4.15-rc3&id=984d91f4c62f64026cbfef51f609971025934cec

> 

> I just don't know how this schema scales with more archs and more platforms

> supported. It's just early days now...

> 

> Regards,

> John

> 

> > jirka

> > 

> > .

> > 

> 

>
John Garry Jan. 2, 2018, 12:07 p.m. UTC | #17
On 16/12/2017 18:47, Andi Kleen wrote:

Hi Andi,

>> Won't this all potentially have a big maintainence cost?

>

> No. It's all auto generated.

>

> The only cost is slightly bigger binary size.

>

> I would hope your event files are auto generated too.

>


No, they're not - we are just manually transcribing the event data from 
the architecture reference manual (for arch-defined events) or internal 
text document (for custom events).

Can you describe how you autogenerate the JSONs? Do you have some 
internal proprietary HW file format describing events, with files 
supplied from HW designer, which you can just translate into a JSON? 
Would the files support deferencing events to improve scalability?

>> I just don't know how this schema scales with more archs and more platforms

>> supported. It's just early days now...

>

> Only perf will get slightly bigger, but memory is not exactly expensive.

>

> In fact, the extra memory won't even be faulted in if it's not used,

> so it's only disk space.


Much appreciated,
John

>

> -Andi

>

> .

>
Andi Kleen Jan. 2, 2018, 5:48 p.m. UTC | #18
> Can you describe how you autogenerate the JSONs? Do you have some internal

> proprietary HW file format describing events, with files supplied from HW

> designer, which you can just translate into a JSON? Would the files support

> deferencing events to improve scalability?


For Intel JSON is an official format, which is maintained for each CPU.
It is automatically generated from an internal database
https://download.01.org/perfmon/

I have some python scripts to convert these Intel JSONs into the perf
format (which has some additional headers, and is split into
different categories, and add metrics).  

They have some Intel specifics, so may not be useful for you. 

There's no support for dereference, each CPU gets its own unique file.

But you could do the a merge simply with the attached script which merges
two JSON files. 

-Andi
#!/usr/bin/python
# merge json event files
# merge-json file1.json file2... > merged.json
import sys
import json

all = []

for fn in sys.argv[1:]:
    jf = json.load(open(fn))
    for n in jf:
	all.append(n)

print json.dumps(all, sort_keys=True, indent=4, separators=(',', ': '))
John Garry Jan. 3, 2018, 12:22 p.m. UTC | #19
On 02/01/2018 17:48, Andi Kleen wrote:
>> Can you describe how you autogenerate the JSONs? Do you have some internal

>> proprietary HW file format describing events, with files supplied from HW

>> designer, which you can just translate into a JSON? Would the files support

>> deferencing events to improve scalability?

>

> For Intel JSON is an official format, which is maintained for each CPU.

> It is automatically generated from an internal database

> https://download.01.org/perfmon/

>

> I have some python scripts to convert these Intel JSONs into the perf

> format (which has some additional headers, and is split into

> different categories, and add metrics).


OK, understood.

Unfortunately I could not see such a database being maintained for ARM 
implementors.

>

> They have some Intel specifics, so may not be useful for you.

>

> There's no support for dereference, each CPU gets its own unique file.


Right.

>

> But you could do the a merge simply with the attached script which merges

> two JSON files.


I assume that you're talking about simply merging the micro architecture 
and the platform specific event JSONS at build time.

If yes, this would not work for us:
- the microarchitecture JSON would contain definitions of all events, 
but there is no architectural method to check if they are implemented
- we need to deal with scenario of non-standard event implementations

But I could update the script to deal with this and add to the build 
(Jirka looked to be ok with the same in jevents, albeit a few caveats).

All the best,
John

>

> -Andi

>
John Garry Jan. 4, 2018, 5:17 p.m. UTC | #20
On 21/12/2017 19:39, Jiri Olsa wrote:
>> Hi Jirka,

>> >

>> > When you say reasonable size for x86, I ran a string duplication finder on

>> > the x86 JSONs and the results show a huge amount of duplication. Please

>> > check this:

>> > https://gist.githubusercontent.com/johnpgarry/68bc87e823ae2ce0f7b475b4e55e5795/raw/f4cea138999d8b34151b9586d733592e01774d7a/x86%2520JSON%2520duplication

>> >

>> > Extract:

>> > "Found a 65 line (311 tokens) duplication in the following files:

>> > Starting at line 100 of

>> > /linux/tools/perf/pmu-events/arch/x86/ivybridge/ivb-metrics.json

>> > Starting at line 100 of

>> > /linux/tools/perf/pmu-events/arch/x86/ivytown/ivt-metrics.json

>> > Starting at line 100 of

>> > /linux/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json

>> > Starting at line 100 of

>> > /linux/tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json

>> > Starting at line 76 of

>> > /linux/tools/perf/pmu-events/arch/x86/jaketown/jkt-metrics.json

>> > Starting at line 100 of

>> > /linux/tools/perf/pmu-events/arch/x86/skylake/skl-metrics.json

>> > Starting at line 76 of

>> > /linux/tools/perf/pmu-events/arch/x86/sandybridge/snb-metrics.json

>> > Starting at line 100 of

>> > /linux/tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json"

>> >



Hi Jirka,

Sorry for the slow reply.

>> > Won't this all potentially have a big maintainence cost?

> as Andi said it's mostly just the disk space,

> which is not big deal

>

> I'm not doing JSON file updates, but I think having

> simple single dir for platform/cpu could save us some

> confusion in future


Understood. But for ARM, which has very standardised architecture 
events, it is good to reduce this event duplication between platforms.

>

> however I won't oppose if you want to add this logic,

> but please:

>   - use the list_head ;-)


Of course

>   - leave the process_one_file function simple

>     and separate the level0 processing


ok, this is how it should look already, albeit a couple of 
process_one_file() modifications. I'll re-check this.

>   - you are using 'EventCode' as an unique ID to find

>     the base, but it's not unique for x86, you'll need

>     to add some other ID scheme that fits to all archs


Right, so you mentioned earlier using a new keyword token to identify 
whether we use the standard event, so we can go his way - ok?

I would also like to mention at this point why I did the event 
pre-processing in jevents, and not a separate script:
- current build does not transverse the arch tree
	- tree transversal for JSON processing is done in jevents
- a script would mean derived objects, which means:
	- makefile changes for derived objects
	- jevents would have to deal with derived objects
- jevents already has support for JSON processing

The advantage of using a script is that we keep the JSON processing in 
jevents simple.

All the best,
John

>

> thanks,

> jirka

>
Jiri Olsa Jan. 8, 2018, 2:08 p.m. UTC | #21
On Thu, Jan 04, 2018 at 05:17:56PM +0000, John Garry wrote:

SNIP

> 

> 

> Hi Jirka,

> 

> Sorry for the slow reply.


np, just got back from holidays anyway ;-)

> 

> > > > Won't this all potentially have a big maintainence cost?

> > as Andi said it's mostly just the disk space,

> > which is not big deal

> > 

> > I'm not doing JSON file updates, but I think having

> > simple single dir for platform/cpu could save us some

> > confusion in future

> 

> Understood. But for ARM, which has very standardised architecture events, it

> is good to reduce this event duplication between platforms.

> 

> > 

> > however I won't oppose if you want to add this logic,

> > but please:

> >   - use the list_head ;-)

> 

> Of course

> 

> >   - leave the process_one_file function simple

> >     and separate the level0 processing

> 

> ok, this is how it should look already, albeit a couple of

> process_one_file() modifications. I'll re-check this.

> 

> >   - you are using 'EventCode' as an unique ID to find

> >     the base, but it's not unique for x86, you'll need

> >     to add some other ID scheme that fits to all archs

> 

> Right, so you mentioned earlier using a new keyword token to identify

> whether we use the standard event, so we can go his way - ok?


yes, something like that

> I would also like to mention at this point why I did the event

> pre-processing in jevents, and not a separate script:

> - current build does not transverse the arch tree

> 	- tree transversal for JSON processing is done in jevents

> - a script would mean derived objects, which means:

> 	- makefile changes for derived objects

> 	- jevents would have to deal with derived objects

> - jevents already has support for JSON processing

> 

> The advantage of using a script is that we keep the JSON processing in

> jevents simple.


I don't mind the extra functionality in jevents as long as the current
one keeps on working and the new one works for all archs ;-)

thanks,
jirka
diff mbox series

Patch

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index a0d489e..a820ed4 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -42,6 +42,7 @@ 
 #include <dirent.h>
 #include <sys/time.h>			/* getrlimit */
 #include <sys/resource.h>		/* getrlimit */
+#include <sys/queue.h>
 #include <ftw.h>
 #include <sys/stat.h>
 #include "jsmn.h"
@@ -366,6 +367,94 @@  static int print_events_table_entry(void *data, char *name, char *event,
 	return 0;
 }
 
+struct event_struct {
+	char *name;
+	char *event;
+	char *desc;
+	char *long_desc;
+	char *pmu;
+	char *unit;
+	char *perpkg;
+	char *metric_expr;
+	char *metric_name;
+	char *metric_group;
+	LIST_ENTRY(event_struct) list;
+	char strings[];
+};
+
+LIST_HEAD(listhead, event_struct) recommended_events;
+
+static int save_recommended_events(void *data, char *name, char *event,
+				    char *desc, char *long_desc,
+				    char *pmu, char *unit, char *perpkg,
+				    char *metric_expr,
+				    char *metric_name, char *metric_group)
+{
+	static int count = 0;
+	char temp[1024];
+	struct event_struct *es;
+	struct stat *sb = data;
+	int len = 0;
+	char *strings;
+
+	/*
+	 * Lazily allocate size of the JSON file to hold the
+	 * strings, which would be more than large enough.
+	 */
+	len = sb->st_size;
+
+	es = malloc(sizeof(*es) + len);
+	if (!es)
+		return -ENOMEM;
+	memset(es, 0, sizeof(*es));
+	LIST_INSERT_HEAD(&recommended_events, es, list);
+
+	strings = &es->strings[0];
+
+	if (name) {
+		es->name = strings;
+		strings += snprintf(strings, len, "%s", name) + 1;
+	}
+	if (event) {
+		es->event = strings;
+		strings += snprintf(strings, len, "%s", event) + 1;
+	}
+	if (desc) {
+		es->desc = strings;
+		strings += snprintf(strings, len, "%s", desc) + 1;
+	}
+	if (long_desc) {
+		es->long_desc = strings;
+		strings += snprintf(strings, len, "%s", long_desc) + 1;
+	}
+	if (pmu) {
+		es->pmu = strings;
+		strings += snprintf(strings, len, "%s", pmu) + 1;
+	}
+	if (unit) {
+		es->unit = strings;
+		strings += snprintf(strings, len, "%s", unit) + 1;
+	}
+	if (perpkg) {
+		es->perpkg = strings;
+		strings += snprintf(strings, len, "%s", perpkg) + 1;
+	}
+	if (metric_expr) {
+		es->metric_expr = strings;
+		strings += snprintf(strings, len, "%s", metric_expr) + 1;
+	}
+	if (metric_name) {
+		es->metric_name = strings;
+		strings += snprintf(strings, len, "%s", metric_name) + 1;
+	}
+	if (metric_group) {
+		es->metric_group = strings;
+		strings += snprintf(strings, len, "%s", metric_group) + 1;
+	}
+
+	return 0;
+}
+
 static void print_events_table_suffix(FILE *outfp)
 {
 	fprintf(outfp, "{\n");
@@ -407,6 +496,61 @@  static char *real_event(const char *name, char *event)
 	return event;
 }
 
+static void fixup_field(char *from, char **to)
+{
+	/*
+	 * If we already had a valid pointer (string), then
+	 * don't allocate a new one, just reuse and overwrite.
+	 */
+	if (!*to)
+		*to = malloc(strlen(from));
+
+	strcpy(*to, from);
+}
+
+static int try_fixup(const char *fn, char *event, char **desc, char **name, char **long_desc, char **pmu, char **filter,
+				char **perpkg, char **unit, char **metric_expr, char **metric_name, char **metric_group)
+{
+	/* try to find matching event from recommended values */
+	struct event_struct *es;
+
+	LIST_FOREACH(es, &recommended_events, list) {
+		if (!strcmp(event, es->event)) {
+			/* now fixup */
+			if (es->desc)
+				fixup_field(es->desc, desc);
+			if (es->name)
+				fixup_field(es->name, name);
+			if (es->long_desc)
+				fixup_field(es->long_desc, long_desc);
+			if (es->pmu)
+				fixup_field(es->pmu, pmu);
+		//	if (event_struct->filter)
+		//		fixup_field(event_struct->filter, filter);
+			if (es->perpkg)
+				fixup_field(es->perpkg, perpkg);
+			if (es->unit)
+				fixup_field(es->unit, unit);
+			if (es->metric_expr)
+				fixup_field(es->metric_expr, metric_expr);
+			if (es->metric_name)
+				fixup_field(es->metric_name, metric_name);
+			if (es->metric_group)
+				fixup_field(es->metric_group, metric_group);
+
+			return 0;
+		}
+	}
+
+	pr_err("%s: could not find matching %s for %s\n", prog, event, fn);
+	return -1;
+}
+
+#define  FREE_MEMORIES \
+		free(event); free(desc); free(name); free(long_desc); \
+		free(extra_desc);  free(pmu); free(filter); free(perpkg); \
+		free(unit); free(metric_expr); free(metric_name);
+
 /* Call func with each event in the json file */
 int json_events(const char *fn,
 	  int (*func)(void *data, char *name, char *event, char *desc,
@@ -551,20 +695,22 @@  int json_events(const char *fn,
 		if (name)
 			fixname(name);
 
+		if (!desc) {
+			/*
+			 * If we have no valid desc, then fixup *all* values from recommended
+			 * by matching the event.
+			 */
+			err = try_fixup(fn, event, &desc, &name, &long_desc, &pmu, &filter, &perpkg, &unit, &metric_expr,
+					&metric_name, &metric_group);
+			if (err) {
+				FREE_MEMORIES
+				goto out_free;
+			}
+		}
+
 		err = func(data, name, real_event(name, event), desc, long_desc,
 			   pmu, unit, perpkg, metric_expr, metric_name, metric_group);
-		free(event);
-		free(desc);
-		free(name);
-		free(long_desc);
-		free(extra_desc);
-		free(pmu);
-		free(filter);
-		free(perpkg);
-		free(unit);
-		free(metric_expr);
-		free(metric_name);
-		free(metric_group);
+		FREE_MEMORIES
 		if (err)
 			break;
 		tok += j;
@@ -776,6 +922,32 @@  static int isLeafDir(const char *fpath)
 	return res;
 }
 
+static int isJsonFile(const char *name)
+{
+	const char *suffix;
+
+	if (strlen(name) < 5)
+		return 0;
+
+	suffix = name + strlen(name) - 5;
+
+	if (strncmp(suffix, ".json", 5) == 0)
+		return 1;
+	return 0;
+}
+
+static int preprocess_level0_files(const char *fpath, const struct stat *sb,
+				int typeflag, struct FTW *ftwbuf)
+{
+	int level	= ftwbuf->level;
+	int is_file = typeflag == FTW_F;
+
+	if (level == 1 && is_file && isJsonFile(fpath))
+		return json_events(fpath, save_recommended_events, (void *)sb);
+
+	return 0;
+}
+
 static int process_one_file(const char *fpath, const struct stat *sb,
 			    int typeflag, struct FTW *ftwbuf)
 {
@@ -806,8 +978,10 @@  static int process_one_file(const char *fpath, const struct stat *sb,
 		 level, sb->st_size, bname, fpath);
 
 	/* base dir */
-	if (level == 0)
-		return 0;
+	if (level == 0) {
+		LIST_INIT(&recommended_events);
+		return nftw(fpath, preprocess_level0_files, get_maxfds(), 0);
+	}
 
 	/* model directory, reset topic */
 	if (level == 1 && is_dir && isLeafDir(fpath)) {
@@ -869,9 +1043,7 @@  static int process_one_file(const char *fpath, const struct stat *sb,
 	 * ignore it. It could be a readme.txt for instance.
 	 */
 	if (is_file) {
-		char *suffix = bname + strlen(bname) - 5;
-
-		if (strncmp(suffix, ".json", 5)) {
+		if (!isJsonFile(bname)) {
 			pr_info("%s: Ignoring file without .json suffix %s\n", prog,
 				fpath);
 			return 0;
@@ -933,6 +1105,7 @@  int main(int argc, char *argv[])
 	const char *output_file;
 	const char *start_dirname;
 	struct stat stbuf;
+	struct event_struct *es1, *es2;
 
 	prog = basename(argv[0]);
 	if (argc < 4) {
@@ -988,6 +1161,14 @@  int main(int argc, char *argv[])
 		goto empty_map;
 	}
 
+	/* Free struct for recommended events */
+	es1 = LIST_FIRST(&recommended_events);
+	while (es1) {
+		es2 = LIST_NEXT(es1, list);
+		free(es1);
+		es1 = es2;
+	}
+
 	if (close_table)
 		print_events_table_suffix(eventsfp);