Messages in this thread | | | Date | Fri, 2 Mar 2012 16:25:48 +0100 | Subject | Re: [PATCH] Perf: bug fix: distinguish between rename and exec | From | Stephane Eranian <> |
| |
On Fri, Mar 2, 2012 at 3:47 PM, Arnaldo Carvalho de Melo <acme@ghostprotocols.net> 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@ghostprotocols.net> 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@ghostprotocols.net> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |