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

[Don Zickus <dzickus@redhat.com>]

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;