From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965397Ab2CBOrm (ORCPT ); Fri, 2 Mar 2012 09:47:42 -0500 Received: from mail-yx0-f174.google.com ([209.85.213.174]:42445 "EHLO mail-yx0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753218Ab2CBOrk (ORCPT ); Fri, 2 Mar 2012 09:47:40 -0500 Authentication-Results: mr.google.com; spf=pass (google.com: domain of arnaldo.melo@gmail.com designates 10.236.153.104 as permitted sender) smtp.mail=arnaldo.melo@gmail.com; dkim=pass header.i=arnaldo.melo@gmail.com Date: Fri, 2 Mar 2012 11:47:28 -0300 From: Arnaldo Carvalho de Melo To: Stephane Eranian Cc: Luigi Semenzato , Peter Zijlstra , Alexander Viro , Paul Mackerras , Ingo Molnar , Andrew Morton , Vasiliy Kulikov , Stephen Wilson , Oleg Nesterov , Tejun Heo , Paul Gortmaker , Andi Kleen , Lucas De Marchi , Greg Kroah-Hartman , "Eric W. Biederman" , "Rafael J. Wysocki" , Frederic Weisbecker , David Ahern , Namhyung Kim , Robert Richter , linux-kernel@vger.kernel.org, sonnyrao@chromium.org, olofj@chromium.org Subject: Re: [PATCH] Perf: bug fix: distinguish between rename and exec Message-ID: <20120302144728.GA14004@infradead.org> References: <1329195360-10699-1-git-send-email-semenzato@chromium.org> <1329310113.2293.72.camel@twins> <20120215134733.GL28614@infradead.org> <20120215174748.GN28614@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, - 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 > 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 > >> 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