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

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

[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 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

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