All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Garry <john.garry@huawei.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: <peterz@infradead.org>, <mingo@redhat.com>, <acme@kernel.org>,
	<mark.rutland@arm.com>, <alexander.shishkin@linux.intel.com>,
	<namhyung@kernel.org>, <kjain@linux.ibm.com>,
	<irogers@google.com>, <yao.jin@linux.intel.com>,
	<yeyunfeng@huawei.com>, <linux-kernel@vger.kernel.org>,
	<linuxarm@huawei.com>, <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] perf jevents: Fix event code for events referencing std arch events
Date: Tue, 13 Oct 2020 09:41:08 +0100	[thread overview]
Message-ID: <9e8e3d2d-d15d-13df-ab97-34df8d81a6a1@huawei.com> (raw)
In-Reply-To: <20201012112419.GJ1099489@krava>

On 12/10/2020 12:24, Jiri Olsa wrote:
> On Mon, Oct 12, 2020 at 12:15:04PM +0100, John Garry wrote:
>> On 12/10/2020 11:54, Jiri Olsa wrote:
>>>> ff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>>>> index 99df41a9543d..e47644cab3fa 100644
>>>> --- a/tools/perf/pmu-events/jevents.c
>>>> +++ b/tools/perf/pmu-events/jevents.c
>>>> @@ -505,20 +505,15 @@ static char *real_event(const char *name, char *event)
>>>>    }
>>>>    static int
>>>> -try_fixup(const char *fn, char *arch_std, unsigned long long eventcode,
>>>> -	  struct json_event *je)
>>>> +try_fixup(const char *fn, char *arch_std, struct json_event *je, char **event)
>>>>    {
>>>>    	/* try to find matching event from arch standard values */
>>>>    	struct event_struct *es;
>>>>    	list_for_each_entry(es, &arch_std_events, list) {
>>>>    		if (!strcmp(arch_std, es->name)) {
>>>> -			if (!eventcode && es->event) {
>>>> -				/* allow EventCode to be overridden */
>>>> -				free(je->event);
>>>> -				je->event = NULL;
>>>> -			}
>>>>    			FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
>>>> +			*event = je->event;
>>> I'm bit rusty on this code, but isn't je->event NULL at this point?
>>
>> je->event should be now assigned from es->event because of
>> FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD):
>>
>> #define TRY_FIXUP_FIELD(field) do { if (es->field && !*field) {\
>> 	*field = strdup(es->field);				\
>> 	if (!*field)						\
>> 		return -ENOMEM;					\
>> } } while (0)
>>
>> And es->event should be set.
> 
> right, thanks
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>
> 
> jirka
> 
> .
> 

"PMU events" testcase was still passing as it does not cover this 
scenario, so I'll look to expand the test to catch such problems.

Thanks,
John


WARNING: multiple messages have this Message-ID (diff)
From: John Garry <john.garry@huawei.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: mark.rutland@arm.com, irogers@google.com, peterz@infradead.org,
	kjain@linux.ibm.com, linux-kernel@vger.kernel.org,
	acme@kernel.org, linuxarm@huawei.com,
	alexander.shishkin@linux.intel.com, yao.jin@linux.intel.com,
	mingo@redhat.com, linux-arm-kernel@lists.infradead.org,
	namhyung@kernel.org, yeyunfeng@huawei.com
Subject: Re: [PATCH] perf jevents: Fix event code for events referencing std arch events
Date: Tue, 13 Oct 2020 09:41:08 +0100	[thread overview]
Message-ID: <9e8e3d2d-d15d-13df-ab97-34df8d81a6a1@huawei.com> (raw)
In-Reply-To: <20201012112419.GJ1099489@krava>

On 12/10/2020 12:24, Jiri Olsa wrote:
> On Mon, Oct 12, 2020 at 12:15:04PM +0100, John Garry wrote:
>> On 12/10/2020 11:54, Jiri Olsa wrote:
>>>> ff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>>>> index 99df41a9543d..e47644cab3fa 100644
>>>> --- a/tools/perf/pmu-events/jevents.c
>>>> +++ b/tools/perf/pmu-events/jevents.c
>>>> @@ -505,20 +505,15 @@ static char *real_event(const char *name, char *event)
>>>>    }
>>>>    static int
>>>> -try_fixup(const char *fn, char *arch_std, unsigned long long eventcode,
>>>> -	  struct json_event *je)
>>>> +try_fixup(const char *fn, char *arch_std, struct json_event *je, char **event)
>>>>    {
>>>>    	/* try to find matching event from arch standard values */
>>>>    	struct event_struct *es;
>>>>    	list_for_each_entry(es, &arch_std_events, list) {
>>>>    		if (!strcmp(arch_std, es->name)) {
>>>> -			if (!eventcode && es->event) {
>>>> -				/* allow EventCode to be overridden */
>>>> -				free(je->event);
>>>> -				je->event = NULL;
>>>> -			}
>>>>    			FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
>>>> +			*event = je->event;
>>> I'm bit rusty on this code, but isn't je->event NULL at this point?
>>
>> je->event should be now assigned from es->event because of
>> FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD):
>>
>> #define TRY_FIXUP_FIELD(field) do { if (es->field && !*field) {\
>> 	*field = strdup(es->field);				\
>> 	if (!*field)						\
>> 		return -ENOMEM;					\
>> } } while (0)
>>
>> And es->event should be set.
> 
> right, thanks
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>
> 
> jirka
> 
> .
> 

"PMU events" testcase was still passing as it does not cover this 
scenario, so I'll look to expand the test to catch such problems.

Thanks,
John


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-10-13  8:44 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-08 15:19 [PATCH] perf jevents: Fix event code for events referencing std arch events John Garry
2020-10-09 11:26 ` kajoljain
2020-10-09 11:38   ` John Garry
2020-10-09 11:38     ` John Garry
2020-10-12 10:54 ` Jiri Olsa
2020-10-12 11:15   ` John Garry
2020-10-12 11:24     ` Jiri Olsa
2020-10-13  8:41       ` John Garry [this message]
2020-10-13  8:41         ` John Garry
2020-10-14 16:49       ` Arnaldo Carvalho de Melo
2020-10-14 17:46         ` John Garry
2020-10-15 12:25           ` Arnaldo Carvalho de Melo
2020-10-15 12:25             ` Arnaldo Carvalho de Melo
2020-10-15 15:42             ` John Garry
2020-10-15 15:42               ` John Garry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9e8e3d2d-d15d-13df-ab97-34df8d81a6a1@huawei.com \
    --to=john.garry@huawei.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=irogers@google.com \
    --cc=jolsa@redhat.com \
    --cc=kjain@linux.ibm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=yao.jin@linux.intel.com \
    --cc=yeyunfeng@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.