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

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.

hi,

Quoted 1 line · Stephane Eranian

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

extra description line ^^^

Quoted 24 lines

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);
}

Quoted 2 lines

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

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 <[email protected]>

tools/perf/util/machine.c +4 -5

index a53cd0b..43cdc0a 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 9 lines · 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.

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

Quoted 54 lines

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 <[email protected]>

tools/perf/util/machine.c +4 -5

index a53cd0b..43cdc0a 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 13 lines · Don Zickus

Quoted 9 lines · 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.

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

Quoted 5 lines

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?

Quoted 6 lines

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?

Quoted 6 lines

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.

Quoted 14 lines · Stephane Eranian

Quoted 13 lines · Don Zickus

Quoted 9 lines · 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.

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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 6 lines

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).

Quoted 2 lines

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.

Quoted 9 lines

Quoted 6 lines

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

Quoted 36 lines · Don Zickus

Quoted 14 lines · Stephane Eranian

Quoted 13 lines · Don Zickus

Quoted 9 lines · 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.

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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 9 lines

Quoted 6 lines

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?

Quoted 4 lines

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?

Quoted 30 lines

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.

Quoted 9 lines

Quoted 6 lines

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

Quoted 37 lines · Stephane Eranian

Quoted 36 lines · Don Zickus

Quoted 14 lines · Stephane Eranian

Quoted 13 lines · Don Zickus

Quoted 9 lines · 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.

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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 9 lines

Quoted 6 lines

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?

Quoted 6 lines

Quoted 4 lines

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

Quoted 30 lines

Quoted 30 lines

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.

Quoted 9 lines

Quoted 6 lines

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

Quoted 9 lines

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.

Quoted 3 lines

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.