Re: [PATCH] Perf: bug fix: distinguish between rename and exec

[Luigi Semenzato]

On Wed, Feb 15, 2012 at 5:47 AM, Arnaldo Carvalho de Melo
<acme@xxxxxxxxxxxxxxxxxx> wrote:
> Em Wed, Feb 15, 2012 at 01:48:33PM +0100, Peter Zijlstra escreveu:
>> I really dislike changing generic code purely for the purpose of
>> instrumentation like this. Better to pull perf_event_comm() out of here
>> if you want to change semantics.
>>
>> Personally I couldn't care less about renames, I think they're a waste
>> of time, so I'm ok with the simple patch moving the perf_event_comm()
>> into setup_new_exec() and possibly renaming it to perf_event_exec().
>>
>> Acme, do you care about renames?
>
> I like your idea of keeping the semantics of PERF_RECORD_COMM and
> introducing a PERF_RECORD_EXEC, just have to think about how to handle
> that in a way that the tools detect that we have PERF_RECORD_EXEC...

I considered this but I don't know how important it is to be backward
compatible.  Adding a new record type makes old "perf report" fail to
parse new perf.data files.  (Unless we pad the new record to a
multiple of 8 bytes, but I don't think we want to go down that path).

If looking forward is more important, I agree a new new record type is
best.  We might want to consider adding a PERF_RECORD_RENAME for
renames, and leaving the COMM record to its historical meaning (exec),
possibly renaming it to PERF_RECORD_EXEC for clarity.  And yes, the
perf instrumentation should not be in set_task_comm(), that's why the
bug exists in the first place.

We might also want to change the parsing of perf.data so that in the
future it is more tolerant of new record types.

>
> Humm, will be yet another fallback for setting an perf_event_attr bit,
> just like with .sample_id_all and .exclude_{guest,host}...
>
> That together with the per class errnos + __strerror() method will allow
> to move all the event creation finally to perf_evlist__open() where all
> this gets nicely hidden away from poor tools.
>
> We can then even have an ui__evlist_perror() method that does all the
> ui__warning calls, etc.
>
> So, yes, from a tooling perspective, I want to be notified of renames
> and being able to stop relying on PERF_RECORD_COMM to call
> map_groups__flush and instead do it at PERF_RECORD_EXEC seems a
> bonus.
>
> - Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/