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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/