Messages in this thread | | | Date | Wed, 23 Apr 2014 14:52:09 +0200 | Subject | Re: [PATCH] perf tools: fix processing of pid/tid for mmap records | From | 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
| |