This patch adds new structure called 'json_event' inside jevents.h
file to improve the callback prototype inside jevent files.
Initially, whenever user want to add new field, they need to update
in all function callback which make it more and more complex with
increased number of parmeters.
With this change, we just need to add it in new structure 'json_event'.
Signed-off-by: Kajol Jain <[email protected]>
---
tools/perf/pmu-events/jevents.c | 188 +++++++++++++-------------------
tools/perf/pmu-events/jevents.h | 28 +++--
2 files changed, 94 insertions(+), 122 deletions(-)
This is the initial prototype to include structure for passing
json fileds. Please, Let me know if this is the right direction
to go forward.
This patch doen't include all the changes, like percore/perchip
field addition. If this looks fine I can send new patch set with
those changes.
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index fa86c5f997cc..606805af69fe 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -318,12 +318,7 @@ static void print_events_table_prefix(FILE *fp, const char *tblname)
close_table = 1;
}
-static int print_events_table_entry(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,
- char *deprecated, char *metric_constraint)
+static int print_events_table_entry(void *data, struct json_event *je)
{
struct perf_entry_data *pd = data;
FILE *outfp = pd->outfp;
@@ -335,30 +330,30 @@ static int print_events_table_entry(void *data, char *name, char *event,
*/
fprintf(outfp, "{\n");
- if (name)
- fprintf(outfp, "\t.name = \"%s\",\n", name);
- if (event)
- fprintf(outfp, "\t.event = \"%s\",\n", event);
- fprintf(outfp, "\t.desc = \"%s\",\n", desc);
+ if (je->name)
+ fprintf(outfp, "\t.name = \"%s\",\n", je->name);
+ if (je->event)
+ fprintf(outfp, "\t.event = \"%s\",\n", je->event);
+ fprintf(outfp, "\t.desc = \"%s\",\n", je->desc);
fprintf(outfp, "\t.topic = \"%s\",\n", topic);
- if (long_desc && long_desc[0])
- fprintf(outfp, "\t.long_desc = \"%s\",\n", long_desc);
- if (pmu)
- fprintf(outfp, "\t.pmu = \"%s\",\n", pmu);
- if (unit)
- fprintf(outfp, "\t.unit = \"%s\",\n", unit);
- if (perpkg)
- fprintf(outfp, "\t.perpkg = \"%s\",\n", perpkg);
- if (metric_expr)
- fprintf(outfp, "\t.metric_expr = \"%s\",\n", metric_expr);
- if (metric_name)
- fprintf(outfp, "\t.metric_name = \"%s\",\n", metric_name);
- if (metric_group)
- fprintf(outfp, "\t.metric_group = \"%s\",\n", metric_group);
- if (deprecated)
- fprintf(outfp, "\t.deprecated = \"%s\",\n", deprecated);
- if (metric_constraint)
- fprintf(outfp, "\t.metric_constraint = \"%s\",\n", metric_constraint);
+ if (je->long_desc && je->long_desc[0])
+ fprintf(outfp, "\t.long_desc = \"%s\",\n", je->long_desc);
+ if (je->pmu)
+ fprintf(outfp, "\t.pmu = \"%s\",\n", je->pmu);
+ if (je->unit)
+ fprintf(outfp, "\t.unit = \"%s\",\n", je->unit);
+ if (je->perpkg)
+ fprintf(outfp, "\t.perpkg = \"%s\",\n", je->perpkg);
+ if (je->metric_expr)
+ fprintf(outfp, "\t.metric_expr = \"%s\",\n", je->metric_expr);
+ if (je->metric_name)
+ fprintf(outfp, "\t.metric_name = \"%s\",\n", je->metric_name);
+ if (je->metric_group)
+ fprintf(outfp, "\t.metric_group = \"%s\",\n", je->metric_group);
+ if (je->deprecated)
+ fprintf(outfp, "\t.deprecated = \"%s\",\n", je->deprecated);
+ if (je->metric_constraint)
+ fprintf(outfp, "\t.metric_constraint = \"%s\",\n", je->metric_constraint);
fprintf(outfp, "},\n");
return 0;
@@ -380,17 +375,17 @@ struct event_struct {
char *metric_constraint;
};
-#define ADD_EVENT_FIELD(field) do { if (field) { \
- es->field = strdup(field); \
+#define ADD_EVENT_FIELD(field) do { if (je->field) { \
+ es->field = strdup(je->field); \
if (!es->field) \
goto out_free; \
} } while (0)
#define FREE_EVENT_FIELD(field) free(es->field)
-#define TRY_FIXUP_FIELD(field) do { if (es->field && !*field) {\
- *field = strdup(es->field); \
- if (!*field) \
+#define TRY_FIXUP_FIELD(field) do { if (es->field && !je->field) {\
+ je->field = strdup(es->field); \
+ if (!je->field) \
return -ENOMEM; \
} } while (0)
@@ -421,11 +416,7 @@ static void free_arch_std_events(void)
}
}
-static int save_arch_std_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,
- char *deprecated, char *metric_constraint)
+static int save_arch_std_events(void *data, struct json_event *je)
{
struct event_struct *es;
@@ -485,11 +476,8 @@ static char *real_event(const char *name, char *event)
}
static int
-try_fixup(const char *fn, char *arch_std, 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, unsigned long long eventcode,
- char **deprecated, char **metric_constraint)
+try_fixup(const char *fn, char *arch_std, unsigned long long eventcode,
+ struct json_event *je)
{
/* try to find matching event from arch standard values */
struct event_struct *es;
@@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
if (!strcmp(arch_std, es->name)) {
if (!eventcode && es->event) {
/* allow EventCode to be overridden */
- free(*event);
- *event = NULL;
+ je->event = NULL;
}
FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
return 0;
@@ -513,13 +500,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
/* 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,
- char *long_desc,
- char *pmu, char *unit, char *perpkg,
- char *metric_expr,
- char *metric_name, char *metric_group,
- char *deprecated, char *metric_constraint),
- void *data)
+ int (*func)(void *data, struct json_event *je),
+ void *data)
{
int err;
size_t size;
@@ -537,24 +519,16 @@ int json_events(const char *fn,
EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array");
tok = tokens + 1;
for (i = 0; i < tokens->size; i++) {
- char *event = NULL, *desc = NULL, *name = NULL;
- char *long_desc = NULL;
char *extra_desc = NULL;
- char *pmu = NULL;
char *filter = NULL;
- char *perpkg = NULL;
- char *unit = NULL;
- char *metric_expr = NULL;
- char *metric_name = NULL;
- char *metric_group = NULL;
- char *deprecated = NULL;
- char *metric_constraint = NULL;
+ struct json_event *je;
char *arch_std = NULL;
unsigned long long eventcode = 0;
struct msrmap *msr = NULL;
jsmntok_t *msrval = NULL;
jsmntok_t *precise = NULL;
jsmntok_t *obj = tok++;
+ je = (struct json_event *)calloc(1, sizeof(struct json_event));
EXPECT(obj->type == JSMN_OBJECT, obj, "expected object");
for (j = 0; j < obj->size; j += 2) {
@@ -570,7 +544,7 @@ int json_events(const char *fn,
"Expected string value");
nz = !json_streq(map, val, "0");
- if (match_field(map, field, nz, &event, val)) {
+ if (match_field(map, field, nz, &je->event, val)) {
/* ok */
} else if (json_streq(map, field, "EventCode")) {
char *code = NULL;
@@ -583,14 +557,14 @@ int json_events(const char *fn,
eventcode |= strtoul(code, NULL, 0) << 21;
free(code);
} else if (json_streq(map, field, "EventName")) {
- addfield(map, &name, "", "", val);
+ addfield(map, &je->name, "", "", val);
} else if (json_streq(map, field, "BriefDescription")) {
- addfield(map, &desc, "", "", val);
- fixdesc(desc);
+ addfield(map, &je->desc, "", "", val);
+ fixdesc(je->desc);
} else if (json_streq(map, field,
"PublicDescription")) {
- addfield(map, &long_desc, "", "", val);
- fixdesc(long_desc);
+ addfield(map, &je->long_desc, "", "", val);
+ fixdesc(je->long_desc);
} else if (json_streq(map, field, "PEBS") && nz) {
precise = val;
} else if (json_streq(map, field, "MSRIndex") && nz) {
@@ -610,34 +584,34 @@ int json_events(const char *fn,
ppmu = field_to_perf(unit_to_pmu, map, val);
if (ppmu) {
- pmu = strdup(ppmu);
+ je->pmu = strdup(ppmu);
} else {
- if (!pmu)
- pmu = strdup("uncore_");
- addfield(map, &pmu, "", "", val);
- for (s = pmu; *s; s++)
+ if (!je->pmu)
+ je->pmu = strdup("uncore_");
+ addfield(map, &je->pmu, "", "", val);
+ for (s = je->pmu; *s; s++)
*s = tolower(*s);
}
- addfield(map, &desc, ". ", "Unit: ", NULL);
- addfield(map, &desc, "", pmu, NULL);
- addfield(map, &desc, "", " ", NULL);
+ addfield(map, &je->desc, ". ", "Unit: ", NULL);
+ addfield(map, &je->desc, "", je->pmu, NULL);
+ addfield(map, &je->desc, "", " ", NULL);
} else if (json_streq(map, field, "Filter")) {
addfield(map, &filter, "", "", val);
} else if (json_streq(map, field, "ScaleUnit")) {
- addfield(map, &unit, "", "", val);
+ addfield(map, &je->unit, "", "", val);
} else if (json_streq(map, field, "PerPkg")) {
- addfield(map, &perpkg, "", "", val);
+ addfield(map, &je->perpkg, "", "", val);
} else if (json_streq(map, field, "Deprecated")) {
- addfield(map, &deprecated, "", "", val);
+ addfield(map, &je->deprecated, "", "", val);
} else if (json_streq(map, field, "MetricName")) {
- addfield(map, &metric_name, "", "", val);
+ addfield(map, &je->metric_name, "", "", val);
} else if (json_streq(map, field, "MetricGroup")) {
- addfield(map, &metric_group, "", "", val);
+ addfield(map, &je->metric_group, "", "", val);
} else if (json_streq(map, field, "MetricConstraint")) {
- addfield(map, &metric_constraint, "", "", val);
+ addfield(map, &je->metric_constraint, "", "", val);
} else if (json_streq(map, field, "MetricExpr")) {
- addfield(map, &metric_expr, "", "", val);
- for (s = metric_expr; *s; s++)
+ addfield(map, &je->metric_expr, "", "", val);
+ for (s = je->metric_expr; *s; s++)
*s = tolower(*s);
} else if (json_streq(map, field, "ArchStdEvent")) {
addfield(map, &arch_std, "", "", val);
@@ -646,7 +620,7 @@ int json_events(const char *fn,
}
/* ignore unknown fields */
}
- if (precise && desc && !strstr(desc, "(Precise Event)")) {
+ if (precise && je->desc && !strstr(je->desc, "(Precise Event)")) {
if (json_streq(map, precise, "2"))
addfield(map, &extra_desc, " ",
"(Must be precise)", NULL);
@@ -655,50 +629,34 @@ int json_events(const char *fn,
"(Precise event)", NULL);
}
snprintf(buf, sizeof buf, "event=%#llx", eventcode);
- addfield(map, &event, ",", buf, NULL);
- if (desc && extra_desc)
- addfield(map, &desc, " ", extra_desc, NULL);
- if (long_desc && extra_desc)
- addfield(map, &long_desc, " ", extra_desc, NULL);
+ addfield(map, &je->event, ",", buf, NULL);
+ if (je->desc && extra_desc)
+ addfield(map, &je->desc, " ", extra_desc, NULL);
+ if (je->long_desc && extra_desc)
+ addfield(map, &je->long_desc, " ", extra_desc, NULL);
if (filter)
- addfield(map, &event, ",", filter, NULL);
+ addfield(map, &je->event, ",", filter, NULL);
if (msr != NULL)
- addfield(map, &event, ",", msr->pname, msrval);
- if (name)
- fixname(name);
+ addfield(map, &je->event, ",", msr->pname, msrval);
+ if (je->name)
+ fixname(je->name);
if (arch_std) {
/*
* An arch standard event is referenced, so try to
* fixup any unassigned values.
*/
- err = try_fixup(fn, arch_std, &event, &desc, &name,
- &long_desc, &pmu, &filter, &perpkg,
- &unit, &metric_expr, &metric_name,
- &metric_group, eventcode,
- &deprecated, &metric_constraint);
+ err = try_fixup(fn, arch_std, eventcode, je);
if (err)
goto free_strings;
}
- err = func(data, name, real_event(name, event), desc, long_desc,
- pmu, unit, perpkg, metric_expr, metric_name,
- metric_group, deprecated, metric_constraint);
+ je->event = real_event(je->name, je->event);
+ err = func(data, je);
free_strings:
- free(event);
- free(desc);
- free(name);
- free(long_desc);
free(extra_desc);
- free(pmu);
free(filter);
- free(perpkg);
- free(deprecated);
- free(unit);
- free(metric_expr);
- free(metric_name);
- free(metric_group);
- free(metric_constraint);
free(arch_std);
+ free(je);
if (err)
break;
diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
index 2afc8304529e..e696edf70e9a 100644
--- a/tools/perf/pmu-events/jevents.h
+++ b/tools/perf/pmu-events/jevents.h
@@ -2,14 +2,28 @@
#ifndef JEVENTS_H
#define JEVENTS_H 1
+#include "pmu-events.h"
+
+struct json_event {
+ char *name;
+ char *event;
+ char *desc;
+ char *topic;
+ char *long_desc;
+ char *pmu;
+ char *unit;
+ char *perpkg;
+ char *metric_expr;
+ char *metric_name;
+ char *metric_group;
+ char *deprecated;
+ char *metric_constraint;
+};
+
int json_events(const char *fn,
- int (*func)(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,
- char *deprecated, char *metric_constraint),
- void *data);
+ int (*func)(void *data, struct json_event *je),
+ void *data);
+
char *get_cpu_str(void);
#ifndef min
--
2.26.2
On 25/08/2020 08:40, Kajol Jain wrote:
> This patch adds new structure called 'json_event' inside jevents.h
> file to improve the callback prototype inside jevent files.
> Initially, whenever user want to add new field, they need to update
> in all function callback which make it more and more complex with
> increased number of parmeters.
> With this change, we just need to add it in new structure 'json_event'.
>
>
> Signed-off-by: Kajol Jain <[email protected]>
> ---
> tools/perf/pmu-events/jevents.c | 188 +++++++++++++-------------------
> tools/perf/pmu-events/jevents.h | 28 +++--
> 2 files changed, 94 insertions(+), 122 deletions(-)
>
> This is the initial prototype to include structure for passing
> json fileds. Please, Let me know if this is the right direction
> to go forward.
> This patch doen't include all the changes, like percore/perchip
> field addition. If this looks fine I can send new patch set with
> those changes.
>
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index fa86c5f997cc..606805af69fe 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -318,12 +318,7 @@ static void print_events_table_prefix(FILE *fp, const char *tblname)
> close_table = 1;
> }
>
> -static int print_events_table_entry(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,
> - char *deprecated, char *metric_constraint)
Right, too many paramaetrs of the same type, so looks reasonable to pass
a struct
[...]
> if (arch_std) {
> /*
> * An arch standard event is referenced, so try to
> * fixup any unassigned values.
> */
> - err = try_fixup(fn, arch_std, &event, &desc, &name,
> - &long_desc, &pmu, &filter, &perpkg,
> - &unit, &metric_expr, &metric_name,
> - &metric_group, eventcode,
> - &deprecated, &metric_constraint);
> + err = try_fixup(fn, arch_std, eventcode, je);
> if (err)
> goto free_strings;
> }
> - err = func(data, name, real_event(name, event), desc, long_desc,
> - pmu, unit, perpkg, metric_expr, metric_name,
> - metric_group, deprecated, metric_constraint);
> + je->event = real_event(je->name, je->event);
> + err = func(data, je);
> free_strings:
> - free(event);
> - free(desc);
> - free(name);
> - free(long_desc);
> free(extra_desc);
> - free(pmu);
> free(filter);
> - free(perpkg);
> - free(deprecated);
> - free(unit);
> - free(metric_expr);
> - free(metric_name);
> - free(metric_group);
> - free(metric_constraint);
> free(arch_std);
> + free(je);
>
> if (err)
> break;
> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
> index 2afc8304529e..e696edf70e9a 100644
> --- a/tools/perf/pmu-events/jevents.h
> +++ b/tools/perf/pmu-events/jevents.h
Somewhat unrelated - this file only seems to be included in jevents.c,
so I don't see why it exists...
> @@ -2,14 +2,28 @@
> #ifndef JEVENTS_H
> #define JEVENTS_H 1
>
> +#include "pmu-events.h"
> +
> +struct json_event {
> + char *name;
> + char *event;
> + char *desc;
> + char *topic;
> + char *long_desc;
> + char *pmu;
> + char *unit;
> + char *perpkg;
> + char *metric_expr;
> + char *metric_name;
> + char *metric_group;
> + char *deprecated;
> + char *metric_constraint;
This looks very much like struct event_struct, so could look to consolidate:
struct event_struct {
struct list_head list;
char *name;
char *event;
char *desc;
char *long_desc;
char *pmu;
char *unit;
char *perpkg;
char *metric_expr;
char *metric_name;
char *metric_group;
char *deprecated;
char *metric_constraint;
};
> +};
> +
> int json_events(const char *fn,
> - int (*func)(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,
> - char *deprecated, char *metric_constraint),
> - void *data);
> + int (*func)(void *data, struct json_event *je),
> + void *data);
> +
> char *get_cpu_str(void);
>
> #ifndef min
>
On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
> This patch adds new structure called 'json_event' inside jevents.h
> file to improve the callback prototype inside jevent files.
> Initially, whenever user want to add new field, they need to update
> in all function callback which make it more and more complex with
> increased number of parmeters.
> With this change, we just need to add it in new structure 'json_event'.
Looks good to me. Thanks.
I wouldn't consolidate with event_struct, these are logically
different layers.
Reviewed-by: Andi Kleen <[email protected]>
On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
SNIP
> {
> /* try to find matching event from arch standard values */
> struct event_struct *es;
> @@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
> if (!strcmp(arch_std, es->name)) {
> if (!eventcode && es->event) {
> /* allow EventCode to be overridden */
> - free(*event);
> - *event = NULL;
> + je->event = NULL;
> }
> FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
> return 0;
> @@ -513,13 +500,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
>
> /* 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,
> - char *long_desc,
> - char *pmu, char *unit, char *perpkg,
> - char *metric_expr,
> - char *metric_name, char *metric_group,
> - char *deprecated, char *metric_constraint),
> - void *data)
> + int (*func)(void *data, struct json_event *je),
> + void *data)
> {
> int err;
> size_t size;
> @@ -537,24 +519,16 @@ int json_events(const char *fn,
> EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array");
> tok = tokens + 1;
> for (i = 0; i < tokens->size; i++) {
> - char *event = NULL, *desc = NULL, *name = NULL;
> - char *long_desc = NULL;
> char *extra_desc = NULL;
> - char *pmu = NULL;
> char *filter = NULL;
> - char *perpkg = NULL;
> - char *unit = NULL;
> - char *metric_expr = NULL;
> - char *metric_name = NULL;
> - char *metric_group = NULL;
> - char *deprecated = NULL;
> - char *metric_constraint = NULL;
> + struct json_event *je;
> char *arch_std = NULL;
> unsigned long long eventcode = 0;
> struct msrmap *msr = NULL;
> jsmntok_t *msrval = NULL;
> jsmntok_t *precise = NULL;
> jsmntok_t *obj = tok++;
> + je = (struct json_event *)calloc(1, sizeof(struct json_event));
hum, you don't check je pointer in here.. but does it need to be allocated?
looks like you could just have je on stack as well..
thanks,
jirka
On 8/26/20 4:27 PM, Jiri Olsa wrote:
> On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
>
> SNIP
>
>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>> index 2afc8304529e..e696edf70e9a 100644
>> --- a/tools/perf/pmu-events/jevents.h
>> +++ b/tools/perf/pmu-events/jevents.h
>> @@ -2,14 +2,28 @@
>> #ifndef JEVENTS_H
>> #define JEVENTS_H 1
>>
>> +#include "pmu-events.h"
>> +
>> +struct json_event {
>> + char *name;
>> + char *event;
>> + char *desc;
>> + char *topic;
>> + char *long_desc;
>> + char *pmu;
>> + char *unit;
>> + char *perpkg;
>> + char *metric_expr;
>> + char *metric_name;
>> + char *metric_group;
>> + char *deprecated;
>> + char *metric_constraint;
>> +};
>> +
>> int json_events(const char *fn,
>> - int (*func)(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,
>> - char *deprecated, char *metric_constraint),
>> - void *data);
>> + int (*func)(void *data, struct json_event *je),
>> + void *data);
>
> please also add typedef for the function,
> it's used in other places as well
Ok I will add that part.
Thanks,
Kajol Jain
>
> thanks,
> jirka
>
On 8/25/20 9:17 PM, Andi Kleen wrote:
> On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
>> This patch adds new structure called 'json_event' inside jevents.h
>> file to improve the callback prototype inside jevent files.
>> Initially, whenever user want to add new field, they need to update
>> in all function callback which make it more and more complex with
>> increased number of parmeters.
>> With this change, we just need to add it in new structure 'json_event'.
>
> Looks good to me. Thanks.
>
> I wouldn't consolidate with event_struct, these are logically
> different layers.
>
> Reviewed-by: Andi Kleen <[email protected]>
>
Hi Andi,
Thanks for reviewing the patch.
Thanks,
Kajol Jain
On Wed, Aug 26, 2020 at 05:02:04PM +0530, kajoljain wrote:
>
>
> On 8/26/20 4:26 PM, Jiri Olsa wrote:
> > On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
> >
> > SNIP
> >
> >> {
> >> /* try to find matching event from arch standard values */
> >> struct event_struct *es;
> >> @@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
> >> if (!strcmp(arch_std, es->name)) {
> >> if (!eventcode && es->event) {
> >> /* allow EventCode to be overridden */
> >> - free(*event);
> >> - *event = NULL;
> >> + je->event = NULL;
> >> }
> >> FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
> >> return 0;
> >> @@ -513,13 +500,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
> >>
> >> /* 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,
> >> - char *long_desc,
> >> - char *pmu, char *unit, char *perpkg,
> >> - char *metric_expr,
> >> - char *metric_name, char *metric_group,
> >> - char *deprecated, char *metric_constraint),
> >> - void *data)
> >> + int (*func)(void *data, struct json_event *je),
> >> + void *data)
> >> {
> >> int err;
> >> size_t size;
> >> @@ -537,24 +519,16 @@ int json_events(const char *fn,
> >> EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array");
> >> tok = tokens + 1;
> >> for (i = 0; i < tokens->size; i++) {
> >> - char *event = NULL, *desc = NULL, *name = NULL;
> >> - char *long_desc = NULL;
> >> char *extra_desc = NULL;
> >> - char *pmu = NULL;
> >> char *filter = NULL;
> >> - char *perpkg = NULL;
> >> - char *unit = NULL;
> >> - char *metric_expr = NULL;
> >> - char *metric_name = NULL;
> >> - char *metric_group = NULL;
> >> - char *deprecated = NULL;
> >> - char *metric_constraint = NULL;
> >> + struct json_event *je;
> >> char *arch_std = NULL;
> >> unsigned long long eventcode = 0;
> >> struct msrmap *msr = NULL;
> >> jsmntok_t *msrval = NULL;
> >> jsmntok_t *precise = NULL;
> >> jsmntok_t *obj = tok++;
> >> + je = (struct json_event *)calloc(1, sizeof(struct json_event));
> >
> > hum, you don't check je pointer in here.. but does it need to be allocated?
> > looks like you could just have je on stack as well..
>
> Hi Jiri,
> Yes I will add check for je pointer here.The reason for allocating memory to 'je' is,
> later we are actually referring one by one to its field and in case if won't allocate memory
> we will get segmentaion fault as otherwise je will be NULL. Please let me know if I am
> getting correct.
I don't see reason why not to use automatic variable in here,
I tried and it seems to work.. below is diff to your changes,
feel free to squash it with your changes
jirka
---
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 606805af69fe..eaac5c126a52 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -521,14 +521,13 @@ int json_events(const char *fn,
for (i = 0; i < tokens->size; i++) {
char *extra_desc = NULL;
char *filter = NULL;
- struct json_event *je;
+ struct json_event je = {};
char *arch_std = NULL;
unsigned long long eventcode = 0;
struct msrmap *msr = NULL;
jsmntok_t *msrval = NULL;
jsmntok_t *precise = NULL;
jsmntok_t *obj = tok++;
- je = (struct json_event *)calloc(1, sizeof(struct json_event));
EXPECT(obj->type == JSMN_OBJECT, obj, "expected object");
for (j = 0; j < obj->size; j += 2) {
@@ -544,7 +543,7 @@ int json_events(const char *fn,
"Expected string value");
nz = !json_streq(map, val, "0");
- if (match_field(map, field, nz, &je->event, val)) {
+ if (match_field(map, field, nz, &je.event, val)) {
/* ok */
} else if (json_streq(map, field, "EventCode")) {
char *code = NULL;
@@ -557,14 +556,14 @@ int json_events(const char *fn,
eventcode |= strtoul(code, NULL, 0) << 21;
free(code);
} else if (json_streq(map, field, "EventName")) {
- addfield(map, &je->name, "", "", val);
+ addfield(map, &je.name, "", "", val);
} else if (json_streq(map, field, "BriefDescription")) {
- addfield(map, &je->desc, "", "", val);
- fixdesc(je->desc);
+ addfield(map, &je.desc, "", "", val);
+ fixdesc(je.desc);
} else if (json_streq(map, field,
"PublicDescription")) {
- addfield(map, &je->long_desc, "", "", val);
- fixdesc(je->long_desc);
+ addfield(map, &je.long_desc, "", "", val);
+ fixdesc(je.long_desc);
} else if (json_streq(map, field, "PEBS") && nz) {
precise = val;
} else if (json_streq(map, field, "MSRIndex") && nz) {
@@ -584,34 +583,34 @@ int json_events(const char *fn,
ppmu = field_to_perf(unit_to_pmu, map, val);
if (ppmu) {
- je->pmu = strdup(ppmu);
+ je.pmu = strdup(ppmu);
} else {
- if (!je->pmu)
- je->pmu = strdup("uncore_");
- addfield(map, &je->pmu, "", "", val);
- for (s = je->pmu; *s; s++)
+ if (!je.pmu)
+ je.pmu = strdup("uncore_");
+ addfield(map, &je.pmu, "", "", val);
+ for (s = je.pmu; *s; s++)
*s = tolower(*s);
}
- addfield(map, &je->desc, ". ", "Unit: ", NULL);
- addfield(map, &je->desc, "", je->pmu, NULL);
- addfield(map, &je->desc, "", " ", NULL);
+ addfield(map, &je.desc, ". ", "Unit: ", NULL);
+ addfield(map, &je.desc, "", je.pmu, NULL);
+ addfield(map, &je.desc, "", " ", NULL);
} else if (json_streq(map, field, "Filter")) {
addfield(map, &filter, "", "", val);
} else if (json_streq(map, field, "ScaleUnit")) {
- addfield(map, &je->unit, "", "", val);
+ addfield(map, &je.unit, "", "", val);
} else if (json_streq(map, field, "PerPkg")) {
- addfield(map, &je->perpkg, "", "", val);
+ addfield(map, &je.perpkg, "", "", val);
} else if (json_streq(map, field, "Deprecated")) {
- addfield(map, &je->deprecated, "", "", val);
+ addfield(map, &je.deprecated, "", "", val);
} else if (json_streq(map, field, "MetricName")) {
- addfield(map, &je->metric_name, "", "", val);
+ addfield(map, &je.metric_name, "", "", val);
} else if (json_streq(map, field, "MetricGroup")) {
- addfield(map, &je->metric_group, "", "", val);
+ addfield(map, &je.metric_group, "", "", val);
} else if (json_streq(map, field, "MetricConstraint")) {
- addfield(map, &je->metric_constraint, "", "", val);
+ addfield(map, &je.metric_constraint, "", "", val);
} else if (json_streq(map, field, "MetricExpr")) {
- addfield(map, &je->metric_expr, "", "", val);
- for (s = je->metric_expr; *s; s++)
+ addfield(map, &je.metric_expr, "", "", val);
+ for (s = je.metric_expr; *s; s++)
*s = tolower(*s);
} else if (json_streq(map, field, "ArchStdEvent")) {
addfield(map, &arch_std, "", "", val);
@@ -620,7 +619,7 @@ int json_events(const char *fn,
}
/* ignore unknown fields */
}
- if (precise && je->desc && !strstr(je->desc, "(Precise Event)")) {
+ if (precise && je.desc && !strstr(je.desc, "(Precise Event)")) {
if (json_streq(map, precise, "2"))
addfield(map, &extra_desc, " ",
"(Must be precise)", NULL);
@@ -629,34 +628,33 @@ int json_events(const char *fn,
"(Precise event)", NULL);
}
snprintf(buf, sizeof buf, "event=%#llx", eventcode);
- addfield(map, &je->event, ",", buf, NULL);
- if (je->desc && extra_desc)
- addfield(map, &je->desc, " ", extra_desc, NULL);
- if (je->long_desc && extra_desc)
- addfield(map, &je->long_desc, " ", extra_desc, NULL);
+ addfield(map, &je.event, ",", buf, NULL);
+ if (je.desc && extra_desc)
+ addfield(map, &je.desc, " ", extra_desc, NULL);
+ if (je.long_desc && extra_desc)
+ addfield(map, &je.long_desc, " ", extra_desc, NULL);
if (filter)
- addfield(map, &je->event, ",", filter, NULL);
+ addfield(map, &je.event, ",", filter, NULL);
if (msr != NULL)
- addfield(map, &je->event, ",", msr->pname, msrval);
- if (je->name)
- fixname(je->name);
+ addfield(map, &je.event, ",", msr->pname, msrval);
+ if (je.name)
+ fixname(je.name);
if (arch_std) {
/*
* An arch standard event is referenced, so try to
* fixup any unassigned values.
*/
- err = try_fixup(fn, arch_std, eventcode, je);
+ err = try_fixup(fn, arch_std, eventcode, &je);
if (err)
goto free_strings;
}
- je->event = real_event(je->name, je->event);
- err = func(data, je);
+ je.event = real_event(je.name, je.event);
+ err = func(data, &je);
free_strings:
free(extra_desc);
free(filter);
free(arch_std);
- free(je);
if (err)
break;
On 8/26/20 4:30 PM, Jiri Olsa wrote:
> On Tue, Aug 25, 2020 at 09:14:11AM +0100, John Garry wrote:
>
> SNIP
>
>>> goto free_strings;
>>> }
>>> - err = func(data, name, real_event(name, event), desc, long_desc,
>>> - pmu, unit, perpkg, metric_expr, metric_name,
>>> - metric_group, deprecated, metric_constraint);
>>> + je->event = real_event(je->name, je->event);
>>> + err = func(data, je);
>>> free_strings:
>>> - free(event);
>>> - free(desc);
>>> - free(name);
>>> - free(long_desc);
>>> free(extra_desc);
>>> - free(pmu);
>>> free(filter);
>>> - free(perpkg);
>>> - free(deprecated);
>>> - free(unit);
>>> - free(metric_expr);
>>> - free(metric_name);
>>> - free(metric_group);
>>> - free(metric_constraint);
>>> free(arch_std);
>>> + free(je);
>>> if (err)
>>> break;
>>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>>> index 2afc8304529e..e696edf70e9a 100644
>>> --- a/tools/perf/pmu-events/jevents.h
>>> +++ b/tools/perf/pmu-events/jevents.h
>>
>> Somewhat unrelated - this file only seems to be included in jevents.c, so I
>> don't see why it exists...
>
> ah right.. I won't mind getting rid of it
Hi John and Jiri
Thanks for reviewing the patch. I can remove this file and add these structure inside jevents.c
Thanks,
Kajol Jain
>
>>> @@ -2,14 +2,28 @@
>>> #ifndef JEVENTS_H
>>> #define JEVENTS_H 1
>>> +#include "pmu-events.h"
>>> +
>>> +struct json_event {
>>> + char *name;
>>> + char *event;
>>> + char *desc;
>>> + char *topic;
>>> + char *long_desc;
>>> + char *pmu;
>>> + char *unit;
>>> + char *perpkg;
>>> + char *metric_expr;
>>> + char *metric_name;
>>> + char *metric_group;
>>> + char *deprecated;
>>> + char *metric_constraint;
>>
>> This looks very much like struct event_struct, so could look to consolidate:
>>
>> struct event_struct {
>> struct list_head list;
>> char *name;
>> char *event;
>> char *desc;
>> char *long_desc;
>> char *pmu;
>> char *unit;
>> char *perpkg;
>> char *metric_expr;
>> char *metric_name;
>> char *metric_group;
>> char *deprecated;
>> char *metric_constraint;
>> };
>
> as Andi said they come from different layers, I think it's
> better to keep them separated even if they share some fields
>
> thanks,
> jirka
>
On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
SNIP
> }
> }
>
> -static int save_arch_std_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,
> - char *deprecated, char *metric_constraint)
> +static int save_arch_std_events(void *data, struct json_event *je)
> {
> struct event_struct *es;
>
> @@ -485,11 +476,8 @@ static char *real_event(const char *name, char *event)
> }
>
> static int
> -try_fixup(const char *fn, char *arch_std, 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, unsigned long long eventcode,
> - char **deprecated, char **metric_constraint)
> +try_fixup(const char *fn, char *arch_std, unsigned long long eventcode,
> + struct json_event *je)
> {
> /* try to find matching event from arch standard values */
> struct event_struct *es;
> @@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
> if (!strcmp(arch_std, es->name)) {
> if (!eventcode && es->event) {
> /* allow EventCode to be overridden */
> - free(*event);
> - *event = NULL;
> + je->event = NULL;
should you free je->event in here?
jirka
On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
> index 2afc8304529e..e696edf70e9a 100644
> --- a/tools/perf/pmu-events/jevents.h
> +++ b/tools/perf/pmu-events/jevents.h
> @@ -2,14 +2,28 @@
> #ifndef JEVENTS_H
> #define JEVENTS_H 1
>
> +#include "pmu-events.h"
> +
> +struct json_event {
> + char *name;
> + char *event;
> + char *desc;
> + char *topic;
> + char *long_desc;
> + char *pmu;
> + char *unit;
> + char *perpkg;
> + char *metric_expr;
> + char *metric_name;
> + char *metric_group;
> + char *deprecated;
> + char *metric_constraint;
> +};
> +
> int json_events(const char *fn,
> - int (*func)(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,
> - char *deprecated, char *metric_constraint),
> - void *data);
> + int (*func)(void *data, struct json_event *je),
> + void *data);
please also add typedef for the function,
it's used in other places as well
thanks,
jirka
On Tue, Aug 25, 2020 at 09:14:11AM +0100, John Garry wrote:
SNIP
> > goto free_strings;
> > }
> > - err = func(data, name, real_event(name, event), desc, long_desc,
> > - pmu, unit, perpkg, metric_expr, metric_name,
> > - metric_group, deprecated, metric_constraint);
> > + je->event = real_event(je->name, je->event);
> > + err = func(data, je);
> > free_strings:
> > - free(event);
> > - free(desc);
> > - free(name);
> > - free(long_desc);
> > free(extra_desc);
> > - free(pmu);
> > free(filter);
> > - free(perpkg);
> > - free(deprecated);
> > - free(unit);
> > - free(metric_expr);
> > - free(metric_name);
> > - free(metric_group);
> > - free(metric_constraint);
> > free(arch_std);
> > + free(je);
> > if (err)
> > break;
> > diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
> > index 2afc8304529e..e696edf70e9a 100644
> > --- a/tools/perf/pmu-events/jevents.h
> > +++ b/tools/perf/pmu-events/jevents.h
>
> Somewhat unrelated - this file only seems to be included in jevents.c, so I
> don't see why it exists...
ah right.. I won't mind getting rid of it
> > @@ -2,14 +2,28 @@
> > #ifndef JEVENTS_H
> > #define JEVENTS_H 1
> > +#include "pmu-events.h"
> > +
> > +struct json_event {
> > + char *name;
> > + char *event;
> > + char *desc;
> > + char *topic;
> > + char *long_desc;
> > + char *pmu;
> > + char *unit;
> > + char *perpkg;
> > + char *metric_expr;
> > + char *metric_name;
> > + char *metric_group;
> > + char *deprecated;
> > + char *metric_constraint;
>
> This looks very much like struct event_struct, so could look to consolidate:
>
> struct event_struct {
> struct list_head list;
> char *name;
> char *event;
> char *desc;
> char *long_desc;
> char *pmu;
> char *unit;
> char *perpkg;
> char *metric_expr;
> char *metric_name;
> char *metric_group;
> char *deprecated;
> char *metric_constraint;
> };
as Andi said they come from different layers, I think it's
better to keep them separated even if they share some fields
thanks,
jirka
On 8/26/20 4:26 PM, Jiri Olsa wrote:
> On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
>
> SNIP
>
>> {
>> /* try to find matching event from arch standard values */
>> struct event_struct *es;
>> @@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
>> if (!strcmp(arch_std, es->name)) {
>> if (!eventcode && es->event) {
>> /* allow EventCode to be overridden */
>> - free(*event);
>> - *event = NULL;
>> + je->event = NULL;
>> }
>> FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
>> return 0;
>> @@ -513,13 +500,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
>>
>> /* 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,
>> - char *long_desc,
>> - char *pmu, char *unit, char *perpkg,
>> - char *metric_expr,
>> - char *metric_name, char *metric_group,
>> - char *deprecated, char *metric_constraint),
>> - void *data)
>> + int (*func)(void *data, struct json_event *je),
>> + void *data)
>> {
>> int err;
>> size_t size;
>> @@ -537,24 +519,16 @@ int json_events(const char *fn,
>> EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array");
>> tok = tokens + 1;
>> for (i = 0; i < tokens->size; i++) {
>> - char *event = NULL, *desc = NULL, *name = NULL;
>> - char *long_desc = NULL;
>> char *extra_desc = NULL;
>> - char *pmu = NULL;
>> char *filter = NULL;
>> - char *perpkg = NULL;
>> - char *unit = NULL;
>> - char *metric_expr = NULL;
>> - char *metric_name = NULL;
>> - char *metric_group = NULL;
>> - char *deprecated = NULL;
>> - char *metric_constraint = NULL;
>> + struct json_event *je;
>> char *arch_std = NULL;
>> unsigned long long eventcode = 0;
>> struct msrmap *msr = NULL;
>> jsmntok_t *msrval = NULL;
>> jsmntok_t *precise = NULL;
>> jsmntok_t *obj = tok++;
>> + je = (struct json_event *)calloc(1, sizeof(struct json_event));
>
> hum, you don't check je pointer in here.. but does it need to be allocated?
> looks like you could just have je on stack as well..
Hi Jiri,
Yes I will add check for je pointer here.The reason for allocating memory to 'je' is,
later we are actually referring one by one to its field and in case if won't allocate memory
we will get segmentaion fault as otherwise je will be NULL. Please let me know if I am
getting correct.
Thanks,
Kajol Jain
>
> thanks,
> jirka
>
On 26/08/2020 12:24, kajoljain wrote:
>
>
> On 8/26/20 4:30 PM, Jiri Olsa wrote:
>> On Tue, Aug 25, 2020 at 09:14:11AM +0100, John Garry wrote:
>>
>> SNIP
>>
>>>> goto free_strings;
>>>> }
>>>> - err = func(data, name, real_event(name, event), desc, long_desc,
>>>> - pmu, unit, perpkg, metric_expr, metric_name,
>>>> - metric_group, deprecated, metric_constraint);
>>>> + je->event = real_event(je->name, je->event);
>>>> + err = func(data, je);
>>>> free_strings:
>>>> - free(event);
>>>> - free(desc);
>>>> - free(name);
>>>> - free(long_desc);
>>>> free(extra_desc);
>>>> - free(pmu);
>>>> free(filter);
>>>> - free(perpkg);
>>>> - free(deprecated);
>>>> - free(unit);
>>>> - free(metric_expr);
>>>> - free(metric_name);
>>>> - free(metric_group);
>>>> - free(metric_constraint);
Hi Kajol Jain,
Do we need to free je->metric_name and the rest still? From a glance,
that memory is still separately alloc'ed in addfield.
>>>> free(arch_std);
>>>> + free(je);
>>>> if (err)
>>>> break;
>>>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>>>> index 2afc8304529e..e696edf70e9a 100644
>>>> --- a/tools/perf/pmu-events/jevents.h
>>>> +++ b/tools/perf/pmu-events/jevents.h
>>>
>>> Somewhat unrelated - this file only seems to be included in jevents.c, so I
>>> don't see why it exists...
>>
>> ah right.. I won't mind getting rid of it
>
> Hi John and Jiri
> Thanks for reviewing the patch. I can remove this file and add these structure inside jevents.c
thanks
>
> Thanks,
> Kajol Jain
>>
>>>> @@ -2,14 +2,28 @@
>>>> #ifndef JEVENTS_H
>>>> #define JEVENTS_H 1
>>>> +#include "pmu-events.h"
>>>> +
>>>> +struct json_event {
>>>> + char *name;
>>>> + char *event;
>>>> + char *desc;
>>>> + char *topic;
>>>> + char *long_desc;
>>>> + char *pmu;
>>>> + char *unit;
>>>> + char *perpkg;
>>>> + char *metric_expr;
>>>> + char *metric_name;
>>>> + char *metric_group;
>>>> + char *deprecated;
>>>> + char *metric_constraint;
>>>
>>> This looks very much like struct event_struct, so could look to consolidate:
>>>
>>> struct event_struct {
>>> struct list_head list;
>>> char *name;
>>> char *event;
>>> char *desc;
>>> char *long_desc;
>>> char *pmu;
>>> char *unit;
>>> char *perpkg;
>>> char *metric_expr;
>>> char *metric_name;
>>> char *metric_group;
>>> char *deprecated;
>>> char *metric_constraint;
>>> };
>>
>> as Andi said they come from different layers, I think it's
>> better to keep them separated even if they share some fields
I was just suggesting to make:
struct event_struct {
struct list_head list;
struct json_event je;
}
No biggie if against this.
Cheers,
John
On 8/26/20 4:27 PM, Jiri Olsa wrote:
> On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
>
> SNIP
>
>> }
>> }
>>
>> -static int save_arch_std_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,
>> - char *deprecated, char *metric_constraint)
>> +static int save_arch_std_events(void *data, struct json_event *je)
>> {
>> struct event_struct *es;
>>
>> @@ -485,11 +476,8 @@ static char *real_event(const char *name, char *event)
>> }
>>
>> static int
>> -try_fixup(const char *fn, char *arch_std, 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, unsigned long long eventcode,
>> - char **deprecated, char **metric_constraint)
>> +try_fixup(const char *fn, char *arch_std, unsigned long long eventcode,
>> + struct json_event *je)
>> {
>> /* try to find matching event from arch standard values */
>> struct event_struct *es;
>> @@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
>> if (!strcmp(arch_std, es->name)) {
>> if (!eventcode && es->event) {
>> /* allow EventCode to be overridden */
>> - free(*event);
>> - *event = NULL;
>> + je->event = NULL;
>
> should you free je->event in here?
Sure, I will add that.
Thanks,
Kajol Jain
>
> jirka
>
On 8/26/20 5:03 PM, John Garry wrote:
> On 26/08/2020 12:24, kajoljain wrote:
>>
>>
>> On 8/26/20 4:30 PM, Jiri Olsa wrote:
>>> On Tue, Aug 25, 2020 at 09:14:11AM +0100, John Garry wrote:
>>>
>>> SNIP
>>>
>>>>> goto free_strings;
>>>>> }
>>>>> - err = func(data, name, real_event(name, event), desc, long_desc,
>>>>> - pmu, unit, perpkg, metric_expr, metric_name,
>>>>> - metric_group, deprecated, metric_constraint);
>>>>> + je->event = real_event(je->name, je->event);
>>>>> + err = func(data, je);
>>>>> free_strings:
>>>>> - free(event);
>>>>> - free(desc);
>>>>> - free(name);
>>>>> - free(long_desc);
>>>>> free(extra_desc);
>>>>> - free(pmu);
>>>>> free(filter);
>>>>> - free(perpkg);
>>>>> - free(deprecated);
>>>>> - free(unit);
>>>>> - free(metric_expr);
>>>>> - free(metric_name);
>>>>> - free(metric_group);
>>>>> - free(metric_constraint);
>
> Hi Kajol Jain,
>
> Do we need to free je->metric_name and the rest still? From a glance, that memory is still separately alloc'ed in addfield.
Hi John,
yes right we should free them as well. Thanks for pointing it, I will update.
Thanks,
Kajol Jain
>
>>>>> free(arch_std);
>>>>> + free(je);
>>>>> if (err)
>>>>> break;
>>>>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>>>>> index 2afc8304529e..e696edf70e9a 100644
>>>>> --- a/tools/perf/pmu-events/jevents.h
>>>>> +++ b/tools/perf/pmu-events/jevents.h
>>>>
>>>> Somewhat unrelated - this file only seems to be included in jevents.c, so I
>>>> don't see why it exists...
>>>
>>> ah right.. I won't mind getting rid of it
>>
>> Hi John and Jiri
>> Thanks for reviewing the patch. I can remove this file and add these structure inside jevents.c
>
> thanks
>
>>
>> Thanks,
>> Kajol Jain
>>>
>>>>> @@ -2,14 +2,28 @@
>>>>> #ifndef JEVENTS_H
>>>>> #define JEVENTS_H 1
>>>>> +#include "pmu-events.h"
>>>>> +
>>>>> +struct json_event {
>>>>> + char *name;
>>>>> + char *event;
>>>>> + char *desc;
>>>>> + char *topic;
>>>>> + char *long_desc;
>>>>> + char *pmu;
>>>>> + char *unit;
>>>>> + char *perpkg;
>>>>> + char *metric_expr;
>>>>> + char *metric_name;
>>>>> + char *metric_group;
>>>>> + char *deprecated;
>>>>> + char *metric_constraint;
>>>>
>>>> This looks very much like struct event_struct, so could look to consolidate:
>>>>
>>>> struct event_struct {
>>>> struct list_head list;
>>>> char *name;
>>>> char *event;
>>>> char *desc;
>>>> char *long_desc;
>>>> char *pmu;
>>>> char *unit;
>>>> char *perpkg;
>>>> char *metric_expr;
>>>> char *metric_name;
>>>> char *metric_group;
>>>> char *deprecated;
>>>> char *metric_constraint;
>>>> };
>>>
>>> as Andi said they come from different layers, I think it's
>>> better to keep them separated even if they share some fields
>
> I was just suggesting to make:
> struct event_struct {
> struct list_head list;
> struct json_event je;
> }
>
> No biggie if against this.
>
> Cheers,
> John
On 8/26/20 5:29 PM, Jiri Olsa wrote:
> On Wed, Aug 26, 2020 at 05:02:04PM +0530, kajoljain wrote:
>>
>>
>> On 8/26/20 4:26 PM, Jiri Olsa wrote:
>>> On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote:
>>>
>>> SNIP
>>>
>>>> {
>>>> /* try to find matching event from arch standard values */
>>>> struct event_struct *es;
>>>> @@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
>>>> if (!strcmp(arch_std, es->name)) {
>>>> if (!eventcode && es->event) {
>>>> /* allow EventCode to be overridden */
>>>> - free(*event);
>>>> - *event = NULL;
>>>> + je->event = NULL;
>>>> }
>>>> FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
>>>> return 0;
>>>> @@ -513,13 +500,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
>>>>
>>>> /* 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,
>>>> - char *long_desc,
>>>> - char *pmu, char *unit, char *perpkg,
>>>> - char *metric_expr,
>>>> - char *metric_name, char *metric_group,
>>>> - char *deprecated, char *metric_constraint),
>>>> - void *data)
>>>> + int (*func)(void *data, struct json_event *je),
>>>> + void *data)
>>>> {
>>>> int err;
>>>> size_t size;
>>>> @@ -537,24 +519,16 @@ int json_events(const char *fn,
>>>> EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array");
>>>> tok = tokens + 1;
>>>> for (i = 0; i < tokens->size; i++) {
>>>> - char *event = NULL, *desc = NULL, *name = NULL;
>>>> - char *long_desc = NULL;
>>>> char *extra_desc = NULL;
>>>> - char *pmu = NULL;
>>>> char *filter = NULL;
>>>> - char *perpkg = NULL;
>>>> - char *unit = NULL;
>>>> - char *metric_expr = NULL;
>>>> - char *metric_name = NULL;
>>>> - char *metric_group = NULL;
>>>> - char *deprecated = NULL;
>>>> - char *metric_constraint = NULL;
>>>> + struct json_event *je;
>>>> char *arch_std = NULL;
>>>> unsigned long long eventcode = 0;
>>>> struct msrmap *msr = NULL;
>>>> jsmntok_t *msrval = NULL;
>>>> jsmntok_t *precise = NULL;
>>>> jsmntok_t *obj = tok++;
>>>> + je = (struct json_event *)calloc(1, sizeof(struct json_event));
>>>
>>> hum, you don't check je pointer in here.. but does it need to be allocated?
>>> looks like you could just have je on stack as well..
>>
>> Hi Jiri,
>> Yes I will add check for je pointer here.The reason for allocating memory to 'je' is,
>> later we are actually referring one by one to its field and in case if won't allocate memory
>> we will get segmentaion fault as otherwise je will be NULL. Please let me know if I am
>> getting correct.
>
> I don't see reason why not to use automatic variable in here,
> I tried and it seems to work.. below is diff to your changes,
> feel free to squash it with your changes
Hi Jiri,
Thanks for the changes, I will update.
Thanks,
Kajol Jain
>
> jirka
>
> ---
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 606805af69fe..eaac5c126a52 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -521,14 +521,13 @@ int json_events(const char *fn,
> for (i = 0; i < tokens->size; i++) {
> char *extra_desc = NULL;
> char *filter = NULL;
> - struct json_event *je;
> + struct json_event je = {};
> char *arch_std = NULL;
> unsigned long long eventcode = 0;
> struct msrmap *msr = NULL;
> jsmntok_t *msrval = NULL;
> jsmntok_t *precise = NULL;
> jsmntok_t *obj = tok++;
> - je = (struct json_event *)calloc(1, sizeof(struct json_event));
>
> EXPECT(obj->type == JSMN_OBJECT, obj, "expected object");
> for (j = 0; j < obj->size; j += 2) {
> @@ -544,7 +543,7 @@ int json_events(const char *fn,
> "Expected string value");
>
> nz = !json_streq(map, val, "0");
> - if (match_field(map, field, nz, &je->event, val)) {
> + if (match_field(map, field, nz, &je.event, val)) {
> /* ok */
> } else if (json_streq(map, field, "EventCode")) {
> char *code = NULL;
> @@ -557,14 +556,14 @@ int json_events(const char *fn,
> eventcode |= strtoul(code, NULL, 0) << 21;
> free(code);
> } else if (json_streq(map, field, "EventName")) {
> - addfield(map, &je->name, "", "", val);
> + addfield(map, &je.name, "", "", val);
> } else if (json_streq(map, field, "BriefDescription")) {
> - addfield(map, &je->desc, "", "", val);
> - fixdesc(je->desc);
> + addfield(map, &je.desc, "", "", val);
> + fixdesc(je.desc);
> } else if (json_streq(map, field,
> "PublicDescription")) {
> - addfield(map, &je->long_desc, "", "", val);
> - fixdesc(je->long_desc);
> + addfield(map, &je.long_desc, "", "", val);
> + fixdesc(je.long_desc);
> } else if (json_streq(map, field, "PEBS") && nz) {
> precise = val;
> } else if (json_streq(map, field, "MSRIndex") && nz) {
> @@ -584,34 +583,34 @@ int json_events(const char *fn,
>
> ppmu = field_to_perf(unit_to_pmu, map, val);
> if (ppmu) {
> - je->pmu = strdup(ppmu);
> + je.pmu = strdup(ppmu);
> } else {
> - if (!je->pmu)
> - je->pmu = strdup("uncore_");
> - addfield(map, &je->pmu, "", "", val);
> - for (s = je->pmu; *s; s++)
> + if (!je.pmu)
> + je.pmu = strdup("uncore_");
> + addfield(map, &je.pmu, "", "", val);
> + for (s = je.pmu; *s; s++)
> *s = tolower(*s);
> }
> - addfield(map, &je->desc, ". ", "Unit: ", NULL);
> - addfield(map, &je->desc, "", je->pmu, NULL);
> - addfield(map, &je->desc, "", " ", NULL);
> + addfield(map, &je.desc, ". ", "Unit: ", NULL);
> + addfield(map, &je.desc, "", je.pmu, NULL);
> + addfield(map, &je.desc, "", " ", NULL);
> } else if (json_streq(map, field, "Filter")) {
> addfield(map, &filter, "", "", val);
> } else if (json_streq(map, field, "ScaleUnit")) {
> - addfield(map, &je->unit, "", "", val);
> + addfield(map, &je.unit, "", "", val);
> } else if (json_streq(map, field, "PerPkg")) {
> - addfield(map, &je->perpkg, "", "", val);
> + addfield(map, &je.perpkg, "", "", val);
> } else if (json_streq(map, field, "Deprecated")) {
> - addfield(map, &je->deprecated, "", "", val);
> + addfield(map, &je.deprecated, "", "", val);
> } else if (json_streq(map, field, "MetricName")) {
> - addfield(map, &je->metric_name, "", "", val);
> + addfield(map, &je.metric_name, "", "", val);
> } else if (json_streq(map, field, "MetricGroup")) {
> - addfield(map, &je->metric_group, "", "", val);
> + addfield(map, &je.metric_group, "", "", val);
> } else if (json_streq(map, field, "MetricConstraint")) {
> - addfield(map, &je->metric_constraint, "", "", val);
> + addfield(map, &je.metric_constraint, "", "", val);
> } else if (json_streq(map, field, "MetricExpr")) {
> - addfield(map, &je->metric_expr, "", "", val);
> - for (s = je->metric_expr; *s; s++)
> + addfield(map, &je.metric_expr, "", "", val);
> + for (s = je.metric_expr; *s; s++)
> *s = tolower(*s);
> } else if (json_streq(map, field, "ArchStdEvent")) {
> addfield(map, &arch_std, "", "", val);
> @@ -620,7 +619,7 @@ int json_events(const char *fn,
> }
> /* ignore unknown fields */
> }
> - if (precise && je->desc && !strstr(je->desc, "(Precise Event)")) {
> + if (precise && je.desc && !strstr(je.desc, "(Precise Event)")) {
> if (json_streq(map, precise, "2"))
> addfield(map, &extra_desc, " ",
> "(Must be precise)", NULL);
> @@ -629,34 +628,33 @@ int json_events(const char *fn,
> "(Precise event)", NULL);
> }
> snprintf(buf, sizeof buf, "event=%#llx", eventcode);
> - addfield(map, &je->event, ",", buf, NULL);
> - if (je->desc && extra_desc)
> - addfield(map, &je->desc, " ", extra_desc, NULL);
> - if (je->long_desc && extra_desc)
> - addfield(map, &je->long_desc, " ", extra_desc, NULL);
> + addfield(map, &je.event, ",", buf, NULL);
> + if (je.desc && extra_desc)
> + addfield(map, &je.desc, " ", extra_desc, NULL);
> + if (je.long_desc && extra_desc)
> + addfield(map, &je.long_desc, " ", extra_desc, NULL);
> if (filter)
> - addfield(map, &je->event, ",", filter, NULL);
> + addfield(map, &je.event, ",", filter, NULL);
> if (msr != NULL)
> - addfield(map, &je->event, ",", msr->pname, msrval);
> - if (je->name)
> - fixname(je->name);
> + addfield(map, &je.event, ",", msr->pname, msrval);
> + if (je.name)
> + fixname(je.name);
>
> if (arch_std) {
> /*
> * An arch standard event is referenced, so try to
> * fixup any unassigned values.
> */
> - err = try_fixup(fn, arch_std, eventcode, je);
> + err = try_fixup(fn, arch_std, eventcode, &je);
> if (err)
> goto free_strings;
> }
> - je->event = real_event(je->name, je->event);
> - err = func(data, je);
> + je.event = real_event(je.name, je.event);
> + err = func(data, &je);
> free_strings:
> free(extra_desc);
> free(filter);
> free(arch_std);
> - free(je);
>
> if (err)
> break;
>