[PATCH] perf tools: fix processing of pid/tid for mmap records

[PATCH] perf tools: fix processing of pid/tid for mmap records

[Stephane Eranian]

perf tools: fix processing of pid/tid for mmap records
    
Mmaps are global to a process (always). Processing them
per-thread was causing some serious issues in case mmaps
would overlap. The overlap fixups would only occur in the
context of the thread which generated the overlapping
mmap. But that was cause issues later on when a sample
from another thread would fall into that overlapping
mmap.

The solution to the problem is to handle ALL mmaps as
occurring in the master thread (pid = tid) and then to
lookup for thread map using pid as the tid argument.
This is how samples are looking up for the thread map
already (notice pid passed twice):
    
int perf_event__preprocess_sample(const union perf_event *event,
                                  struct machine *machine,
                                  struct addr_location *al,
                                  struct perf_sample *sample)
{
        struct thread *thread = machine__findnew_thread(machine, sample->pid,
                                                        sample->pid);
}
    
Without this fix, some samples in overlapping regions
may not be symbolized.
    
Signed-off-by: Stephane Eranian <eranian@google.com>

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index a53cd0b..43cdc0a 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1025,9 +1025,9 @@ int machine__process_mmap2_event(struct machine *machine,
 			goto out_problem;
 		return 0;
 	}
-
+	/* only look by pid for mmap events */
 	thread = machine__findnew_thread(machine, event->mmap2.pid,
-					event->mmap2.tid);
+					event->mmap2.pid);
 	if (thread == NULL)
 		goto out_problem;
 
@@ -1073,9 +1073,9 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
 			goto out_problem;
 		return 0;
 	}
-
+	/* only look by pid for mmap events */
 	thread = machine__findnew_thread(machine, event->mmap.pid,
-					 event->mmap.tid);
+					 event->mmap.pid);
 	if (thread == NULL)
 		goto out_problem;
 

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Jiri Olsa]

hi,

On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
> perf tools: fix processing of pid/tid for mmap records

extra description line ^^^

>     
> Mmaps are global to a process (always). Processing them
> per-thread was causing some serious issues in case mmaps
> would overlap. The overlap fixups would only occur in the
> context of the thread which generated the overlapping
> mmap. But that was cause issues later on when a sample
> from another thread would fall into that overlapping
> mmap.
> 
> The solution to the problem is to handle ALL mmaps as
> occurring in the master thread (pid = tid) and then to
> lookup for thread map using pid as the tid argument.
> This is how samples are looking up for the thread map
> already (notice pid passed twice):
>     
> int perf_event__preprocess_sample(const union perf_event *event,
>                                   struct machine *machine,
>                                   struct addr_location *al,
>                                   struct perf_sample *sample)
> {
>         struct thread *thread = machine__findnew_thread(machine, sample->pid,
>                                                         sample->pid);
> }
>     
> Without this fix, some samples in overlapping regions
> may not be symbolized.

could you please take a look on following patchset:
http://marc.info/?l=linux-kernel&m=139749074531132&w=2

this makes the map groups shared within the process,
so it should fix above issue as well

thanks,
jirka

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Stephane Eranian]

Jiri,

On Tue, Apr 22, 2014 at 3:32 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> hi,
>
> On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
>> perf tools: fix processing of pid/tid for mmap records
>
> extra description line ^^^
>
>>
>> Mmaps are global to a process (always). Processing them
>> per-thread was causing some serious issues in case mmaps
>> would overlap. The overlap fixups would only occur in the
>> context of the thread which generated the overlapping
>> mmap. But that was cause issues later on when a sample
>> from another thread would fall into that overlapping
>> mmap.
>>
>> The solution to the problem is to handle ALL mmaps as
>> occurring in the master thread (pid = tid) and then to
>> lookup for thread map using pid as the tid argument.
>> This is how samples are looking up for the thread map
>> already (notice pid passed twice):
>>
>> int perf_event__preprocess_sample(const union perf_event *event,
>>                                   struct machine *machine,
>>                                   struct addr_location *al,
>>                                   struct perf_sample *sample)
>> {
>>         struct thread *thread = machine__findnew_thread(machine, sample->pid,
>>                                                         sample->pid);
>> }
>>
>> Without this fix, some samples in overlapping regions
>> may not be symbolized.
>
> could you please take a look on following patchset:
> http://marc.info/?l=linux-kernel&m=139749074531132&w=2
>
> this makes the map groups shared within the process,
> so it should fix above issue as well
>
It could probably solve my problem. I will try it out.

I am wondering why the tid was taken into consideration in the first place
when looking for maps? why was pid not enough?

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Don Zickus]

On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
> perf tools: fix processing of pid/tid for mmap records
>     
> Mmaps are global to a process (always). Processing them
> per-thread was causing some serious issues in case mmaps
> would overlap. The overlap fixups would only occur in the
> context of the thread which generated the overlapping
> mmap. But that was cause issues later on when a sample
> from another thread would fall into that overlapping
> mmap.

eh?  You are basically reverting my patch for a similar problem. :-)

I was running a large multi-threading program (specjbb) and the threads
were not being seperated into their own map'd areas.  So either I had to
lump all threads in to the same pid space or make the change you are
reverting.

The problem I had with the double pid (as you propose), I would later look
up samples based on pid/tid and there would be _no_ map available because
it was created as a pid/pid pair.  As a result, our c2c program would drop
thousands of samples on the floor (because there was no mapping for the
data address to get the major/minor/inode info).

Now I modified our c2c program to lookup samples as pid/pid but now the
maps lost tid info, and I had to hack around that by carrying the tid info
in a private struct.

Hopefully Jiri's thread work using pointers will solve both our problems.
:-)  But I can't ack your patch because it will break my work :-(

Cheers,
Don


> 
> The solution to the problem is to handle ALL mmaps as
> occurring in the master thread (pid = tid) and then to
> lookup for thread map using pid as the tid argument.
> This is how samples are looking up for the thread map
> already (notice pid passed twice):
>     
> int perf_event__preprocess_sample(const union perf_event *event,
>                                   struct machine *machine,
>                                   struct addr_location *al,
>                                   struct perf_sample *sample)
> {
>         struct thread *thread = machine__findnew_thread(machine, sample->pid,
>                                                         sample->pid);
> }
>     
> Without this fix, some samples in overlapping regions
> may not be symbolized.
>     
> Signed-off-by: Stephane Eranian <eranian@google.com>
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index a53cd0b..43cdc0a 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1025,9 +1025,9 @@ int machine__process_mmap2_event(struct machine *machine,
>  			goto out_problem;
>  		return 0;
>  	}
> -
> +	/* only look by pid for mmap events */
>  	thread = machine__findnew_thread(machine, event->mmap2.pid,
> -					event->mmap2.tid);
> +					event->mmap2.pid);
>  	if (thread == NULL)
>  		goto out_problem;
>  
> @@ -1073,9 +1073,9 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
>  			goto out_problem;
>  		return 0;
>  	}
> -
> +	/* only look by pid for mmap events */
>  	thread = machine__findnew_thread(machine, event->mmap.pid,
> -					 event->mmap.tid);
> +					 event->mmap.pid);
>  	if (thread == NULL)
>  		goto out_problem;
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Stephane Eranian]

On Tue, Apr 22, 2014 at 9:40 PM, Don Zickus <dzickus@redhat.com> wrote:
> On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
>> perf tools: fix processing of pid/tid for mmap records
>>
>> Mmaps are global to a process (always). Processing them
>> per-thread was causing some serious issues in case mmaps
>> would overlap. The overlap fixups would only occur in the
>> context of the thread which generated the overlapping
>> mmap. But that was cause issues later on when a sample
>> from another thread would fall into that overlapping
>> mmap.
>
> eh?  You are basically reverting my patch for a similar problem. :-)
>
> I was running a large multi-threading program (specjbb) and the threads
> were not being seperated into their own map'd areas.  So either I had to
> lump all threads in to the same pid space or make the change you are
> reverting.
>
I don't understand your problem. The address space is shared by ALL
threads of a process. The mmaps are always shared by all threads?

> The problem I had with the double pid (as you propose), I would later look
> up samples based on pid/tid and there would be _no_ map available because
> it was created as a pid/pid pair.  As a result, our c2c program would drop
> thousands of samples on the floor (because there was no mapping for the
> data address to get the major/minor/inode info).

That is your problem. You should only lookup mapping baseds on pid only
not pid/tid. Why do you need tid?

>
> Now I modified our c2c program to lookup samples as pid/pid but now the
> maps lost tid info, and I had to hack around that by carrying the tid info
> in a private struct.
>
If you need the tid, then it is okay to carry it on the side. I believe mmap
lookups should ONLY use pid. That is how the address space of a process
is contructed.

> Hopefully Jiri's thread work using pointers will solve both our problems.
> :-)  But I can't ack your patch because it will break my work :-(
>
> Cheers,
> Don
>
>
>>
>> The solution to the problem is to handle ALL mmaps as
>> occurring in the master thread (pid = tid) and then to
>> lookup for thread map using pid as the tid argument.
>> This is how samples are looking up for the thread map
>> already (notice pid passed twice):
>>
>> int perf_event__preprocess_sample(const union perf_event *event,
>>                                   struct machine *machine,
>>                                   struct addr_location *al,
>>                                   struct perf_sample *sample)
>> {
>>         struct thread *thread = machine__findnew_thread(machine, sample->pid,
>>                                                         sample->pid);
>> }
>>
>> Without this fix, some samples in overlapping regions
>> may not be symbolized.
>>
>> Signed-off-by: Stephane Eranian <eranian@google.com>
>>
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index a53cd0b..43cdc0a 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1025,9 +1025,9 @@ int machine__process_mmap2_event(struct machine *machine,
>>                       goto out_problem;
>>               return 0;
>>       }
>> -
>> +     /* only look by pid for mmap events */
>>       thread = machine__findnew_thread(machine, event->mmap2.pid,
>> -                                     event->mmap2.tid);
>> +                                     event->mmap2.pid);
>>       if (thread == NULL)
>>               goto out_problem;
>>
>> @@ -1073,9 +1073,9 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
>>                       goto out_problem;
>>               return 0;
>>       }
>> -
>> +     /* only look by pid for mmap events */
>>       thread = machine__findnew_thread(machine, event->mmap.pid,
>> -                                      event->mmap.tid);
>> +                                      event->mmap.pid);
>>       if (thread == NULL)
>>               goto out_problem;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Don Zickus]

On Tue, Apr 22, 2014 at 09:50:17PM +0200, Stephane Eranian wrote:
> On Tue, Apr 22, 2014 at 9:40 PM, Don Zickus <dzickus@redhat.com> wrote:
> > On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
> >> perf tools: fix processing of pid/tid for mmap records
> >>
> >> Mmaps are global to a process (always). Processing them
> >> per-thread was causing some serious issues in case mmaps
> >> would overlap. The overlap fixups would only occur in the
> >> context of the thread which generated the overlapping
> >> mmap. But that was cause issues later on when a sample
> >> from another thread would fall into that overlapping
> >> mmap.
> >
> > eh?  You are basically reverting my patch for a similar problem. :-)
> >
> > I was running a large multi-threading program (specjbb) and the threads
> > were not being seperated into their own map'd areas.  So either I had to
> > lump all threads in to the same pid space or make the change you are
> > reverting.
> >
> I don't understand your problem. The address space is shared by ALL
> threads of a process. The mmaps are always shared by all threads?

Sure.


> 
> > The problem I had with the double pid (as you propose), I would later look
> > up samples based on pid/tid and there would be _no_ map available because
> > it was created as a pid/pid pair.  As a result, our c2c program would drop
> > thousands of samples on the floor (because there was no mapping for the
> > data address to get the major/minor/inode info).
> 
> That is your problem. You should only lookup mapping baseds on pid only
> not pid/tid. Why do you need tid?

Because the function is machine__findnew_thread not
machine__findnew_map.  I want the thread info.  That includes the tid,
comm and any other thread specific info.  :-)

The problem was the thread maps were not being shared internally, leading
to your problem and my problem.

Jiri fixed that.  So now I can request a thread struct based on a pid/tid
and the map groups all point to the same one created by the pid.  As it
should.


> 
> >
> > Now I modified our c2c program to lookup samples as pid/pid but now the
> > maps lost tid info, and I had to hack around that by carrying the tid info
> > in a private struct.
> >
> If you need the tid, then it is okay to carry it on the side. I believe mmap
> lookups should ONLY use pid. That is how the address space of a process
> is contructed.

The tid is stored in the thread struct.  So if I have a pointer to the
thread struct, I shouldn't have to carry the tid on the side.  That was
the point. :-)  In fact the thread struct is inside the hist_entry which
is referenced by struct hist.  So now I can easily sort and display tid
without carrying anything on the side.

But that only works if the pid/tid mappings are setup correctly.  Which I
believe Jiri has done with his recent patchset.

Cheers,
Don

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Stephane Eranian]

On Tue, Apr 22, 2014 at 10:45 PM, Don Zickus <dzickus@redhat.com> wrote:
> On Tue, Apr 22, 2014 at 09:50:17PM +0200, Stephane Eranian wrote:
>> On Tue, Apr 22, 2014 at 9:40 PM, Don Zickus <dzickus@redhat.com> wrote:
>> > On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
>> >> perf tools: fix processing of pid/tid for mmap records
>> >>
>> >> Mmaps are global to a process (always). Processing them
>> >> per-thread was causing some serious issues in case mmaps
>> >> would overlap. The overlap fixups would only occur in the
>> >> context of the thread which generated the overlapping
>> >> mmap. But that was cause issues later on when a sample
>> >> from another thread would fall into that overlapping
>> >> mmap.
>> >
>> > eh?  You are basically reverting my patch for a similar problem. :-)
>> >
>> > I was running a large multi-threading program (specjbb) and the threads
>> > were not being seperated into their own map'd areas.  So either I had to
>> > lump all threads in to the same pid space or make the change you are
>> > reverting.
>> >
>> I don't understand your problem. The address space is shared by ALL
>> threads of a process. The mmaps are always shared by all threads?
>
> Sure.
>
>
>>
>> > The problem I had with the double pid (as you propose), I would later look
>> > up samples based on pid/tid and there would be _no_ map available because
>> > it was created as a pid/pid pair.  As a result, our c2c program would drop
>> > thousands of samples on the floor (because there was no mapping for the
>> > data address to get the major/minor/inode info).
>>
>> That is your problem. You should only lookup mapping baseds on pid only
>> not pid/tid. Why do you need tid?
>
> Because the function is machine__findnew_thread not
> machine__findnew_map.  I want the thread info.  That includes the tid,
> comm and any other thread specific info.  :-)
>
But then, the two call should be separated: one to get the mapping, one to get
the thread info. What's wrong with this approach?

> The problem was the thread maps were not being shared internally, leading
> to your problem and my problem.
>
> Jiri fixed that.  So now I can request a thread struct based on a pid/tid
> and the map groups all point to the same one created by the pid.  As it
> should.
>
>
>>
>> >
>> > Now I modified our c2c program to lookup samples as pid/pid but now the
>> > maps lost tid info, and I had to hack around that by carrying the tid info
>> > in a private struct.
>> >
>> If you need the tid, then it is okay to carry it on the side. I believe mmap
>> lookups should ONLY use pid. That is how the address space of a process
>> is contructed.
>
> The tid is stored in the thread struct.  So if I have a pointer to the
> thread struct, I shouldn't have to carry the tid on the side.  That was
> the point. :-)  In fact the thread struct is inside the hist_entry which
> is referenced by struct hist.  So now I can easily sort and display tid
> without carrying anything on the side.
>
> But that only works if the pid/tid mappings are setup correctly.  Which I
> believe Jiri has done with his recent patchset.
>
> Cheers,
> Don

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Don Zickus]

On Wed, Apr 23, 2014 at 02:52:09PM +0200, Stephane Eranian wrote:
> On Tue, Apr 22, 2014 at 10:45 PM, Don Zickus <dzickus@redhat.com> wrote:
> > On Tue, Apr 22, 2014 at 09:50:17PM +0200, Stephane Eranian wrote:
> >> On Tue, Apr 22, 2014 at 9:40 PM, Don Zickus <dzickus@redhat.com> wrote:
> >> > On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
> >> >> perf tools: fix processing of pid/tid for mmap records
> >> >>
> >> >> Mmaps are global to a process (always). Processing them
> >> >> per-thread was causing some serious issues in case mmaps
> >> >> would overlap. The overlap fixups would only occur in the
> >> >> context of the thread which generated the overlapping
> >> >> mmap. But that was cause issues later on when a sample
> >> >> from another thread would fall into that overlapping
> >> >> mmap.
> >> >
> >> > eh?  You are basically reverting my patch for a similar problem. :-)
> >> >
> >> > I was running a large multi-threading program (specjbb) and the threads
> >> > were not being seperated into their own map'd areas.  So either I had to
> >> > lump all threads in to the same pid space or make the change you are
> >> > reverting.
> >> >
> >> I don't understand your problem. The address space is shared by ALL
> >> threads of a process. The mmaps are always shared by all threads?
> >
> > Sure.
> >
> >
> >>
> >> > The problem I had with the double pid (as you propose), I would later look
> >> > up samples based on pid/tid and there would be _no_ map available because
> >> > it was created as a pid/pid pair.  As a result, our c2c program would drop
> >> > thousands of samples on the floor (because there was no mapping for the
> >> > data address to get the major/minor/inode info).
> >>
> >> That is your problem. You should only lookup mapping baseds on pid only
> >> not pid/tid. Why do you need tid?
> >
> > Because the function is machine__findnew_thread not
> > machine__findnew_map.  I want the thread info.  That includes the tid,
> > comm and any other thread specific info.  :-)
> >
> But then, the two call should be separated: one to get the mapping, one to get
> the thread info. What's wrong with this approach?

I don't agree with the two call approach as it wouldn't be an intutive
api.

What currently happens is the mmap events are generated to creating
mappings.  Most mappings are for the pids.  Originally none were for the
tids.  But as you said earlier, the tid mappings should share the pid
mappings, so there should be no need to create tid mappings explicitly.

But that implies the the thread struct can find the pid mappings.  Today
it can not.  Currently, it looks to see if it has an explicit thread
mapping created by an mmap event (which it most likely will not).

As a result, the thread lookup based on tid finds a NULL mapping and in my
case the sample is useless because I don't have major/minor/inode info.

However..... with Jiri's changes, when looking up a thread struct with a
pid/tid combo, _if_ the tid mapping is NULL, Jiri's patch attempts to
locate the pid mapping and return a pointer to it, which the the thread
struct saves for future reference.

That is probably the behaviour we both expect.

Jiri's patch takes all mmap events as pid maps and creates an initial map
for them.  All future threads that don't have mappings will trying to use
their pid's mapping as a fallback option.

Because it is all pointer based now, updating the pid mappings
automatically update all the thread mappings too.

I believe this approach will is the right one and makes sense.  Do you
think that works for you?

Cheers,
Don

> 
> > The problem was the thread maps were not being shared internally, leading
> > to your problem and my problem.
> >
> > Jiri fixed that.  So now I can request a thread struct based on a pid/tid
> > and the map groups all point to the same one created by the pid.  As it
> > should.
> >
> >
> >>
> >> >
> >> > Now I modified our c2c program to lookup samples as pid/pid but now the
> >> > maps lost tid info, and I had to hack around that by carrying the tid info
> >> > in a private struct.
> >> >
> >> If you need the tid, then it is okay to carry it on the side. I believe mmap
> >> lookups should ONLY use pid. That is how the address space of a process
> >> is contructed.
> >
> > The tid is stored in the thread struct.  So if I have a pointer to the
> > thread struct, I shouldn't have to carry the tid on the side.  That was
> > the point. :-)  In fact the thread struct is inside the hist_entry which
> > is referenced by struct hist.  So now I can easily sort and display tid
> > without carrying anything on the side.
> >
> > But that only works if the pid/tid mappings are setup correctly.  Which I
> > believe Jiri has done with his recent patchset.
> >
> > Cheers,
> > Don

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Stephane Eranian]

Don,

On Wed, Apr 23, 2014 at 3:30 PM, Don Zickus <dzickus@redhat.com> wrote:
> On Wed, Apr 23, 2014 at 02:52:09PM +0200, Stephane Eranian wrote:
>> On Tue, Apr 22, 2014 at 10:45 PM, Don Zickus <dzickus@redhat.com> wrote:
>> > On Tue, Apr 22, 2014 at 09:50:17PM +0200, Stephane Eranian wrote:
>> >> On Tue, Apr 22, 2014 at 9:40 PM, Don Zickus <dzickus@redhat.com> wrote:
>> >> > On Mon, Apr 21, 2014 at 06:06:55PM +0200, Stephane Eranian wrote:
>> >> >> perf tools: fix processing of pid/tid for mmap records
>> >> >>
>> >> >> Mmaps are global to a process (always). Processing them
>> >> >> per-thread was causing some serious issues in case mmaps
>> >> >> would overlap. The overlap fixups would only occur in the
>> >> >> context of the thread which generated the overlapping
>> >> >> mmap. But that was cause issues later on when a sample
>> >> >> from another thread would fall into that overlapping
>> >> >> mmap.
>> >> >
>> >> > eh?  You are basically reverting my patch for a similar problem. :-)
>> >> >
>> >> > I was running a large multi-threading program (specjbb) and the threads
>> >> > were not being seperated into their own map'd areas.  So either I had to
>> >> > lump all threads in to the same pid space or make the change you are
>> >> > reverting.
>> >> >
>> >> I don't understand your problem. The address space is shared by ALL
>> >> threads of a process. The mmaps are always shared by all threads?
>> >
>> > Sure.
>> >
>> >
>> >>
>> >> > The problem I had with the double pid (as you propose), I would later look
>> >> > up samples based on pid/tid and there would be _no_ map available because
>> >> > it was created as a pid/pid pair.  As a result, our c2c program would drop
>> >> > thousands of samples on the floor (because there was no mapping for the
>> >> > data address to get the major/minor/inode info).
>> >>
>> >> That is your problem. You should only lookup mapping baseds on pid only
>> >> not pid/tid. Why do you need tid?
>> >
>> > Because the function is machine__findnew_thread not
>> > machine__findnew_map.  I want the thread info.  That includes the tid,
>> > comm and any other thread specific info.  :-)
>> >
>> But then, the two call should be separated: one to get the mapping, one to get
>> the thread info. What's wrong with this approach?
>
> I don't agree with the two call approach as it wouldn't be an intutive
> api.
>
> What currently happens is the mmap events are generated to creating
> mappings.  Most mappings are for the pids.  Originally none were for the
> tids.  But as you said earlier, the tid mappings should share the pid
> mappings, so there should be no need to create tid mappings explicitly.
>
> But that implies the the thread struct can find the pid mappings.  Today
> it can not.  Currently, it looks to see if it has an explicit thread
> mapping created by an mmap event (which it most likely will not).
>
> As a result, the thread lookup based on tid finds a NULL mapping and in my
> case the sample is useless because I don't have major/minor/inode info.
>
> However..... with Jiri's changes, when looking up a thread struct with a
> pid/tid combo, _if_ the tid mapping is NULL, Jiri's patch attempts to
> locate the pid mapping and return a pointer to it, which the the thread
> struct saves for future reference.
>
> That is probably the behaviour we both expect.
>
> Jiri's patch takes all mmap events as pid maps and creates an initial map
> for them.  All future threads that don't have mappings will trying to use
> their pid's mapping as a fallback option.
>
> Because it is all pointer based now, updating the pid mappings
> automatically update all the thread mappings too.
>
> I believe this approach will is the right one and makes sense.  Do you
> think that works for you?
>
I just tried Jiri's 5 patches and my JIT patches, and it does work correctly.
My Java samples are correctly symbolized. So looks like we have a solution
that works for both of us though it seems complicated. But I can live with it.
Thanks Jiri.

> Cheers,
> Don
>
>>
>> > The problem was the thread maps were not being shared internally, leading
>> > to your problem and my problem.
>> >
>> > Jiri fixed that.  So now I can request a thread struct based on a pid/tid
>> > and the map groups all point to the same one created by the pid.  As it
>> > should.
>> >
>> >
>> >>
>> >> >
>> >> > Now I modified our c2c program to lookup samples as pid/pid but now the
>> >> > maps lost tid info, and I had to hack around that by carrying the tid info
>> >> > in a private struct.
>> >> >
>> >> If you need the tid, then it is okay to carry it on the side. I believe mmap
>> >> lookups should ONLY use pid. That is how the address space of a process
>> >> is contructed.
>> >
>> > The tid is stored in the thread struct.  So if I have a pointer to the
>> > thread struct, I shouldn't have to carry the tid on the side.  That was
>> > the point. :-)  In fact the thread struct is inside the hist_entry which
>> > is referenced by struct hist.  So now I can easily sort and display tid
>> > without carrying anything on the side.
>> >
>> > But that only works if the pid/tid mappings are setup correctly.  Which I
>> > believe Jiri has done with his recent patchset.
>> >
>> > Cheers,
>> > Don

Re: [PATCH] perf tools: fix processing of pid/tid for mmap records

[Jiri Olsa]

On Wed, Apr 23, 2014 at 05:06:21PM +0200, Stephane Eranian wrote:

SNIP

> > That is probably the behaviour we both expect.
> >
> > Jiri's patch takes all mmap events as pid maps and creates an initial map
> > for them.  All future threads that don't have mappings will trying to use
> > their pid's mapping as a fallback option.
> >
> > Because it is all pointer based now, updating the pid mappings
> > automatically update all the thread mappings too.
> >
> > I believe this approach will is the right one and makes sense.  Do you
> > think that works for you?
> >
> I just tried Jiri's 5 patches and my JIT patches, and it does work correctly.
> My Java samples are correctly symbolized. So looks like we have a solution
> that works for both of us though it seems complicated. But I can live with it.
> Thanks Jiri.

can I take this for your Tested-by?

thanks,
jirka