linux-kernel - Re: [PATCH 35/40] perf record: Synthesize COMM event for a command line workload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150520005625.GG22713@sejong>
Date:	Wed, 20 May 2015 09:56:25 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	Jiri Olsa <jolsa@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	LKML <linux-kernel@...r.kernel.org>,
	David Ahern <dsahern@...il.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Andi Kleen <andi@...stfloor.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 35/40] perf record: Synthesize COMM event for a command
 line workload

On Tue, May 19, 2015 at 09:22:10PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, May 20, 2015 at 08:56:58AM +0900, Namhyung Kim escreveu:
> > On Tue, May 19, 2015 at 05:18:54PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, May 19, 2015 at 09:49:03PM +0200, Jiri Olsa escreveu:
> > > > > > Humm, you're thinking about where you managed to reproduce the problem,
> > > > > > I am thinking outside indexing, etc, i.e. by definition we either enable
> > > > > > the event before we fork, so that we get the PERF_RECORD_FORK/COMM or we
> > > > > > synthesize it either from /proc or directly (preferred) if we decide to
> > > > > > do it after the fork/exec, right?
> 
> > > > > But as I said before, later COMM event will override thread->comm to a
> > > > > proper string as long as it can find a matching thread.  So I think it
> > > > > has no problem in the current code.
>  
> > > > I can see the issue in the current script code and the patch cured it ;-)
>  
> > > Exactly, this is my point, this is not something new :-)
>  
> > Ah, okay.  The perf script shows samples before processing comm events
> > while perf report shows after processing all events.
> 
> I.e. 'perf script' behaves like 'perf trace' and 'perf top'. 'perf
> report' is the odd one out, and I think it should be not, i.e. you
> should try to think more about the non 'report' use cases when thinking
> about how to improve report :-)

I'll keep it in mind.

> 
> But I digress, lets get back to the question at hand...
>  
> > But to move it under generic place like perf_evlist__{prepare,start}_
> > workload(), it seems we need to pass an additional callback and data.
> 
> Only if you want to do it with perf_event__synthesize_comm(). I
> suggested writing a new synthesize routine that doesn't parses /proc, as
> we have all that we need, no?

Agreed.

> 
> I think that just doing something like:
> 
> 	thread = machine__findnew_thread(evlist->workload.pid, evlist->workload.pid);
> 	if (thread)
> 		thread__set_comm(thread, argv[0], timestamp);
> 
> Should be enough, no? I.e. no need for setting up a PERF_RECORD_FORK and
> a PERF_RECORD_COMM, read /proc, etc, just do it directly with the info
> we used to do the fork in perf_evlist__prepare_workload(), etc.

For non-record use case it'd be enough.  But for record, it needs to
synthesize/write an event to data file so that perf report can
recognize it later.  That's why I think it needs callback.

Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/