linux-kernel - Re: [PATCH 12/38] perf tools: Introduce thread__comm

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] [day] [month] [year] [list]
Message-ID: <20150310065528.GE943@sejong>
Date:	Tue, 10 Mar 2015 15:55:28 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Jiri Olsa <jolsa@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Stephane Eranian <eranian@...gle.com>,
	Andi Kleen <andi@...stfloor.org>,
	David Ahern <dsahern@...il.com>
Subject: Re: [PATCH 12/38] perf tools: Introduce thread__comm_time() helpers

Hi Arnaldo,

On Fri, Mar 06, 2015 at 09:48:05AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 06, 2015 at 01:38:06PM +0900, Namhyung Kim escreveu:
> > On Thu, Mar 05, 2015 at 05:08:56PM +0100, Frederic Weisbecker wrote:
> > > On Wed, Mar 04, 2015 at 09:02:55AM +0900, Namhyung Kim wrote:
> > > > On Tue, Mar 03, 2015 at 05:28:40PM +0100, Frederic Weisbecker wrote:
> > > > > On Tue, Mar 03, 2015 at 12:07:24PM +0900, Namhyung Kim wrote:
> > > > > > When data file indexing is enabled, it processes all task, comm and mmap
> > > > > > events first and then goes to the sample events.  So all it sees is the
> > > > > > last comm of a thread although it has information at the time of sample.
> 
> > > > > > Sort thread's comm by time so that it can find appropriate comm at the
> > > > > > sample time.  The thread__comm_time() will mostly work even if
> > > > > > PERF_SAMPLE_TIME bit is off since in that case, sample->time will be
> > > > > > -1 so it'll take the last comm anyway.
> 
> > > > > > +++ b/tools/perf/util/thread.c
> > > > > > @@ -103,6 +103,21 @@ struct comm *thread__exec_comm(const struct thread *thread)
> > > > > >  	return last;
> > > > > > +struct comm *thread__comm_time(const struct thread *thread, u64 timestamp)
> 
> > > > > Usually thread__comm_foo() would suggest that we return the "foo" from a thread comm.
> > > > > For example thread__comm_len() returns the len of the last thread comm.
> > > > > thread__comm_str() returns the string of the last thread comm.
> 
> > > > Ah, okay.
> 
> > > I mean, that's just an impression, others may have a different one :o)
> > 
> > Right.  Although I agree with your idea of function naming, I'm not
> > sure it's worth changing every function call site for this - and for
> > similar machine__find(new)_thread()_time.
> 
> So you need to find a thread using a new search key, namely (pid, tid,
> time), is that right?

Right.

> 
> I.e. you process all the meta events so that you can have the lifetime
> events for the whole session, right?

Exactly. :)


> 
> If that is the case, then I think we should keep the existing functions
> as is, for tools not yet converted to this new way of processing
> samples, be it just because there was no time yet to do it or because we
> may find that it is not adequate for some tools, and introduce the new
> API, that takes the time as part of the key, so, in addition to:
> 
> 
> 	machine__find_thread
> 	machine__findnew_thread
> 
> namely:
> 
> 	machine__find_thread_by_time
> 	machine__findnew_thread_by_time
> 
> But thread__comm_time() for me is to get the time of most recent comm
> for that thread. I.e. I agree with Frédéric, if what you want is to
> _find_ the comm for a specific key, i.e. time, then I think it should
> be:
> 
>         thread__find_comm_by_time(thread, tstamp)

I'm okay with it, will change!


> 
> Ah, how do you manage the thread rbtree? First a search by pid/tid and
> then another by time? Or put it all together and then
> machine__find_thread becomes just a:
> 
> struct thread *machine__find_thread(machine, pid, tid)
> {
> 	return machine__find_thread_by_time(machine, pid, tid, -1);
> }
> 
> And it returns the most recent thread for that pid/tid?

Yeah, but there's an issue with multi-thread access.  The find_thread
API is good but findnew_thread API is a problem since we need to
protect creation of new thread from multiple concurrent accesses.  So
basically it needs a lock but since it's in a hotpath it'd hurt the
performance IMHO.

So I separate the API - existing functions should be called in a
serial execution only.  The new APIs allow concurrent accesses in that
it searches (current) thread tree first and then searches dead thread
tree if not found.  Almost all of thread search should success at this
stage.  Since these operations don't change anything, they don't
require locking.

For occasional search failure, it introduced a new tree (called
'missing tree') protected with a lock.  Threads are added only if
they're not found in the normal (current + dead) tree.


> 
> I.e.  machine__remove_thread() becomes a noop for such use cases and we
> never delete a struct thread instance?

Right.  I just converted to keep removed threads in a rbtree as
they can be quite large in a long session.

Thanks,
Namhyung


> 
> - Arnaldo
> --
> 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/
--
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/