[PATCH] perf jevents: Fix event code for events referencing std arch events

[PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

The event code for events referencing std arch events is incorrectly
evaluated in json_events().

The issue is that je.event is evaluated properly from try_fixup(), but
later NULLified from the real_event() call, as "event" may be NULL.

Fix by setting "event" same je.event in try_fixup().

Also remove support for overwriting event code for events using std arch
events, as it is not used.

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

diff --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;
 			return 0;
 		}
 	}
@@ -678,7 +673,7 @@ static int json_events(const char *fn,
 			 * 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, &je, &event);
 			if (err)
 				goto free_strings;
 		}
-- 
2.26.2

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[kajoljain]

On 10/8/20 8:49 PM, John Garry wrote:
> The event code for events referencing std arch events is incorrectly
> evaluated in json_events().
> 
> The issue is that je.event is evaluated properly from try_fixup(), but
> later NULLified from the real_event() call, as "event" may be NULL.
> 
> Fix by setting "event" same je.event in try_fixup().
> 
> Also remove support for overwriting event code for events using std arch
> events, as it is not used.

Patch looks good to me. Not sure if any adding any example with this issue
is helpful.

Reviewed-By: Kajol Jain<kjain@linux.ibm.com>

Thanks,
Kajol Jain
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> 
> diff --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;
>  			return 0;
>  		}
>  	}
> @@ -678,7 +673,7 @@ static int json_events(const char *fn,
>  			 * 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, &je, &event);
>  			if (err)
>  				goto free_strings;
>  		}
> 

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

On 09/10/2020 12:26, kajoljain wrote:
> 
> On 10/8/20 8:49 PM, John Garry wrote:
>> The event code for events referencing std arch events is incorrectly
>> evaluated in json_events().
>>
>> The issue is that je.event is evaluated properly from try_fixup(), but
>> later NULLified from the real_event() call, as "event" may be NULL.
>>
>> Fix by setting "event" same je.event in try_fixup().
>>
>> Also remove support for overwriting event code for events using std arch
>> events, as it is not used.
> Patch looks good to me. Not sure if any adding any example with this issue
> is helpful.

So we get something like this in pmu-events.c:

{
         .name = "l1d_cache_wb_victim",
         .event = "event=0x0",
[...]
},

But event should "event=0x46".

I can add this if Arnaldo wants a v2.

> 
> Reviewed-By: Kajol Jain<kjain@linux.ibm.com>

thanks

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

On 09/10/2020 12:26, kajoljain wrote:
> 
> On 10/8/20 8:49 PM, John Garry wrote:
>> The event code for events referencing std arch events is incorrectly
>> evaluated in json_events().
>>
>> The issue is that je.event is evaluated properly from try_fixup(), but
>> later NULLified from the real_event() call, as "event" may be NULL.
>>
>> Fix by setting "event" same je.event in try_fixup().
>>
>> Also remove support for overwriting event code for events using std arch
>> events, as it is not used.
> Patch looks good to me. Not sure if any adding any example with this issue
> is helpful.

So we get something like this in pmu-events.c:

{
         .name = "l1d_cache_wb_victim",
         .event = "event=0x0",
[...]
},

But event should "event=0x46".

I can add this if Arnaldo wants a v2.

> 
> Reviewed-By: Kajol Jain<kjain@linux.ibm.com>

thanks


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

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[Jiri Olsa]

On Thu, Oct 08, 2020 at 11:19:28PM +0800, John Garry wrote:
> The event code for events referencing std arch events is incorrectly
> evaluated in json_events().
> 
> The issue is that je.event is evaluated properly from try_fixup(), but
> later NULLified from the real_event() call, as "event" may be NULL.
> 
> Fix by setting "event" same je.event in try_fixup().
> 
> Also remove support for overwriting event code for events using std arch
> events, as it is not used.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> 
> diff --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?

jirka

>  			return 0;
>  		}
>  	}
> @@ -678,7 +673,7 @@ static int json_events(const char *fn,
>  			 * 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, &je, &event);
>  			if (err)
>  				goto free_strings;
>  		}
> -- 
> 2.26.2
> 

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

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.

Thanks,
John

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[Jiri Olsa]

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

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

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

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

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

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[Arnaldo Carvalho de Melo]

Em Mon, Oct 12, 2020 at 01:24:19PM +0200, Jiri Olsa escreveu:
> 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>

Ok, applied, please consider adding a Fixes tag next time.

- Arnaldo

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

On 14/10/2020 17:49, Arnaldo Carvalho de Melo wrote:
> Ok, applied,

Thanks

>please consider adding a Fixes tag next time.
> 

Can do if it helps, but I only thought it appropriate when fixing 
something merged to mainline.

John

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[Arnaldo Carvalho de Melo]

Em Wed, Oct 14, 2020 at 06:46:12PM +0100, John Garry escreveu:
> On 14/10/2020 17:49, Arnaldo Carvalho de Melo wrote:
> > Ok, applied,
> 
> Thanks
> 
> > please consider adding a Fixes tag next time.
> > 
> 
> Can do if it helps, but I only thought it appropriate when fixing something
> merged to mainline.

Please do, I think it appropriate in all cases, people doing backports
may decide to pick something and if it has some subtle issue this can be
automatically checked for by looking at any later Fixes for that
specific cset.

I decided not to do any rebase on perf/core, so those Fixes will remain
valid.

- Arnaldo

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[Arnaldo Carvalho de Melo]

Em Wed, Oct 14, 2020 at 06:46:12PM +0100, John Garry escreveu:
> On 14/10/2020 17:49, Arnaldo Carvalho de Melo wrote:
> > Ok, applied,
> 
> Thanks
> 
> > please consider adding a Fixes tag next time.
> > 
> 
> Can do if it helps, but I only thought it appropriate when fixing something
> merged to mainline.

Please do, I think it appropriate in all cases, people doing backports
may decide to pick something and if it has some subtle issue this can be
automatically checked for by looking at any later Fixes for that
specific cset.

I decided not to do any rebase on perf/core, so those Fixes will remain
valid.

- Arnaldo

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

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

> 
> I decided not to do any rebase on perf/core, so those Fixes will remain
> valid.

Ah, ok. Then that should be fine wrt fixes tags.

Cheers,
John

Re: [PATCH] perf jevents: Fix event code for events referencing std arch events

[John Garry]

> 
> I decided not to do any rebase on perf/core, so those Fixes will remain
> valid.

Ah, ok. Then that should be fine wrt fixes tags.

Cheers,
John

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