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

[Stephane Eranian]

On Fri, Mar 2, 2012 at 3:47 PM, Arnaldo Carvalho de Melo
<acme@xxxxxxxxxxxxxxxxxx> wrote:
> Em Fri, Mar 02, 2012 at 02:44:37PM +0100, Stephane Eranian escreveu:
>> Did we come to an agreement on this problem?
>>
>> Seems like we do want to distinguish exec from renames, as perf
>> David's example with top.
>> In Luigi's patch, the distinction is made via the header->misc
>> bitmask. Now, I see people have
>> proposed a new record type: PERF_RECORD_COMM vs. PERF_RECORD_EXEC. This would
>> also work but it would require a lot more changes in the tool, i.e.,
>
> He is working on it, already submitted a RFC and will follow up on lkml
> soon,
>
On which solution is he working on?

> - Arnaldo
>
>> you have to process a new
>> record type. What's wrong with the header->misc approach?
>>
>>
>> On Wed, Feb 15, 2012 at 6:47 PM, Arnaldo Carvalho de Melo
>> <acme@xxxxxxxxxxxxxxxxxx> wrote:
>> > Em Wed, Feb 15, 2012 at 09:07:47AM -0800, Luigi Semenzato escreveu:
>> >> 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
>> >
>> > Hey, old perf record would still see the PERF_RECORD_COMM, i.e. we would
>> > continue asking for PERF_RECORD_COMM in new versions. Together with
>> > PERF_RECORD_EXEC.
>> >
>> >> 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
>> >
>> > PERF_RECORD_COMM is good enough, well, it always was confusing for most
>> > people that asked "hey, that means an EXEC, right?"
>> >
>> > First thing pople think is "hey, this is when it sets the thread COMM,
>> > right?"
>> >
>> >> 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.
>> >
>> > Yes, what is the behaviour now? Lemme see... Well, difficult, I'm barely
>> > reading this, just after magnifying it, dilated pupils two hours ago,
>> > grrr
>> >
>> > WiÄl check later, but IIRC it just warns and skips the record, right?
>> >
>> >> > 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/