Re: [RFC] perf/jevents: Add new structure to pass json fields.
From: kajoljain
Date: Thu Aug 27 2020 - 09:06:47 EST
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;
>