Re: [PATCH 1/2] perf: fix synthesizing mmaps for threads

Don Zickus
2014-03-05T17:29:08Z

On Wed, Mar 05, 2014 at 05:58:57PM +0100, Jiri Olsa wrote:
> On Wed, Feb 26, 2014 at 10:45:26AM -0500, Don Zickus wrote:
> > Currently if a process creates a bunch of threads using pthread_create
> > and then perf is run in system_wide mode, the mmaps for those threads
> > are not captured with a synthesized mmap event.
> >
> > The reason is those threads are not visible when walking the /proc/
> > directory looking for /proc/<pid>/maps files. Instead they are discovered
> > using the /proc/<pid>/tasks file (which the synthesized comm event uses).
> >
> > This causes problems when a program is trying to map a data address to a
> > tid. Because the tid has no maps, the event is dropped. Changing the program
> > to look up using the pid instead of the tid, finds the correct maps but creates
> > ugly hacks in the program to carry the correct tid around.
>
> hm, 2 hacks comes to my mind ;-)
>
> 1) share 'struct thread::mg' among thread group (pid)
>
> 2) or lookup the thread group leader if we find out we are
> not the leader and dont have the map info (attached)
>
> your change makes the process map info (same info) being duplicated
> for all threads (eventhough it's probably not that much bytes wasted)
>
> I think I'd prefer ad 1) ... the patch for ad 2) assumes there's
> always thread group leader (which might not be the case always?)
> also 'thread->pid_' handling seems troubled
>
> I dont have code solution for 1), maybe you've already cover that
> and considered it hacky.. just throwing ideas ;-)

It doesn't matter to me. :-) The c2c tool needs this to work correctly
otherwise the analysis is wrong when profiling the system with the app
already running with lots of threads (ie databases).

Well I shouldn't say the analysis is wrong, it just wrongly attributes the
pid being responsible for all the problems when it could be some garbage
collection thread running to frequently.

Cheers,
Don

>
> jirka
>
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index b0f3ca8..c428186 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -654,9 +654,17 @@ void thread__find_addr_map(struct thread *thread,
> enum map_type type, u64 addr,
> struct addr_location *al)
> {
> - struct map_groups *mg = &thread->mg;
> + struct map_groups *mg;
> bool load_map = false;
>
> + if (thread->tid != thread->pid_) {
> + thread = machine__findnew_thread(machine, thread->pid_, thread->pid_);
> + if (!thread)
> + return;
> + }
> +
> + mg = &thread->mg;
> +
> al->machine = machine;
> al->thread = thread;
> al->addr = addr;