Re: [Patch] Losing many samples when profiling multiple JVMs

[William C. <wc...@re...>]

On 06/22/2015 07:19 PM, 大平怜 wrote:
> Thanks for the investigation, Will,
> 
> Unfortunately, your patch does not solve my problem. In my previous email, I mentioned only the situation where multiple processes have different-sized anon regions that happen to have the same starting address. However, there is another situation, in which a process expands its anon region (by mremap(2)?) with the same starting address.  Since your patch does not check the ending address, oprofile incorrectly reuses an existing operf_mmap struct representing a smaller anon region.  I think in any way the checking of the ending address is inevitable.
> 

Hi Rei,

Thanks for the testing.  I originally thought that the problem was due to different processes having overlapping maps.  However, from the testing it sounds like there are multiple maps for the same process and the smaller map is removing the correct mapping from the list.  Are the small sized maps always zero-sized?  The existing code to compute mapping->end_addr is a bit odd in that it special cases the end_addr as 0 for zero sized mappings because region is start_addr<=addr<=end_addr and there isn't a great way of indicating nothing is in the region. The patch is matching that behavior.  Would it be better if the event->mmap.len is 0 just skip making the entry? That would have the plus of reducing the number of entries to scan through.

Could you run operf with the following option on the problem testcase to see if there are any lines with "end addr: 0"?

 operf -V convert <command_being_measured>

>> However, what happens when two different processes have anon regions that have the same starting address and are the same size?
> 
> In that case, they share the same operf_mmap struct.  Is there any problem in it?

For read only regions of regular executable code this is okay.  However, if the anon memory is for different processes, they are likely to have different contents and it isn't a good idea to assume they are the same.  That what the patch I sent was trying to address, keep them separate so the contents don't get confused.

> 
> I have just git-cloned oprofile-test but could not figure out how to run it.  Could you help me create a test case?

I can help you get a test written for oprofile-test.  For the time being it would be great if you just have some steps that you used to create the problem.  It doesn't have the be an actual script.  Just a list of commands and some text description what is expected when it doesn't work and when it does work.  From earlier email this problem is only on some versions of the jvm and architectures also make a note of that in the writeup.  I will be at the Red hat summit in Boston, MA most of this week, so I may not be able to do too much on this the next couple of days.


-Will

> 
> 
> Thanks,
> Rei Odaira
>  
> 
> 2015-06-17 8:55 GMT-05:00 William Cohen <wc...@re... <mailto:wc...@re...>>:
> 
>     On 06/16/2015 04:24 PM, William Cohen wrote:
>     > On 06/12/2015 04:49 PM, 大平怜 wrote:
>     >> Hi,
>     >>
>     >> Anyone has any suggestions?  I hope my patch is included in OProfile-1.1.
>     >>
>     >>
>     >> Thanks,
>     >> Rei Odaira
>     >>
>     >> 2015-05-31 22:00 GMT-05:00 大平怜 <rei...@gm... <mailto:rei...@gm...> <mailto:rei...@gm... <mailto:rei...@gm...>>>:
>     >>
>     >>     Hi,
>     >>
>     >>     I found when profiling multiple JVMs, many samples that should have hit JITted methods were lost due to no permanent mapping. This was because OProfile did not have correct operf_mmap for the JITted code anon regions of some of the JVMs. This error can happen as follows:
>     >>
>     >>     (1) __handle_mmap_event() creates a new operf_mmap for a small anon region of a JVM.
>     >>     (2) When another JVM happens to allocate its anon region at the same starting virtual address but with a larger size, __handle_mmap_event() first searches all_images_map.
>     >>     (3) __handle_mmap_event() finds the previously created operf_mmap in all_images_map and reuses it, because it checks only filename and start_addr.
>     >>     (4) As a result, when a sample hits a JITted method that was generated between the incorrect end_addr and the actual end address, __get_operf_trans() cannot find an appropriate mapping.
>     >>
>     >>     The root cause of this problem is that __handle_mmap_event() does not check end_addr when searching all_images_map. This should work for a normal file, which should have the same size when mapped to different processes, but not for anon regions, which can have different sizes in different processes. The attached patch will add this check.
>     >>
>     >
>     > Hi Rei,
>     >
>     > The patch sounds like a step in the right direction.  However, what happens when two different processes have anon regions that have the same starting address and are the same size?  It  would be preferable to use the pid of struct mmap_event to determine whether the maps for the anonymous regions are for the same process, but the struct operf_mmap doesn't keep the pid information.  Maybe the struct operf_mmap should be modified and the code include that for the anonymous memory maps.  Use pid value of 0 for mappings for multiple pids.
>     >
>     >>     More fundamentally, I am wondering what the purpose of all_images_region is. I guess it is to reuse operf_mmap structures for more than one processes. The file names in all_images_map are stored in absolute paths, but they are compared against a file base name in __handle_mmap_event(). As a result, this comparison succeeds only for anon and [vdso]. Is this intended behavior?
>     >>
>     >
>     > I haven't done that much looking in this area of the oprofile code.  I did some code archaeology with gitk to see how to code evolved and see if there was anything in the commits that described what was being addressed.  It looks like the following patch put in that particular code that your patch is modifying.  The goal is to avoid having multiple mappings for different processes with the same binaries/mappings.  There needs to be some checking earlier to see whether the mapping is something that is unique to the process (an anonymous mapping) and avoid the incorrect matching of pseudo file names ("//anon" , "/anon_hugepage").
>     >
>     >
>     >
>     > Author: Maynard Johnson <may...@us... <mailto:may...@us...>>  2012-03-27 19:03:45
>     > Committer: Maynard Johnson <may...@us... <mailto:may...@us...>>  2012-03-27 19:03:45
>     > Parent: ed9f73aab1adb7ee368027a2532649baf867a45d (Fix problem when passed app name is of the form '<subdir>/<app_name>')
>     > Child:  f7f71a580b65f5fedcd1bbd94825838a94a0945a (Fix timing-related problems with app name)
>     > Branches: master, remotes/origin/master
>     > Follows: RELEASE_0_9_7
>     > Precedes: RELEASE_0_9_8
>     >
>     >     Minor bug fixes and performance improvement for system-wide mode.
>     >
>     >     The operf_sfile hashing algorithm was modified from legacy hashing
>     >     to use either a build-id or a checksum.  I had not yet implemented the
>     >     getting the build-id, so was falling back to the checksum.  Getting
>     >     the checksum (via gelf_checksum) was terribly expensive, especially
>     >     noticeable when running with the system-wide option.  So I spent
>     >     some time comparing using a build-id (obtained from the binary file's
>     >     NT_GNU_BUILD_ID note) for the hashing algorithm versus simply hashing
>     >     on a substring of the image name.  The simple string hash was as
>     >     effective as the build-id method.  Both were way better than the checksum
>     >     technique. In the end, I removed all of the checksum and build-id stuff,
>     >     and settled on the image name string hash method for finding operf_sfiles.
>     >
>     >     Aside from the above big performance improvement described above, I also
>     >     created a global multimap for holding operf_mmap objects.  The operf_mmap
>     >     encapsulates an MMAP event.  In system-wide mode, we get lots of MMAP events
>     >     for the same libraries being used by many different processes, so it was
>     >     logical to create just one operf_mmap object per unique binary.
>     >
>     >
>     > -Will
> 
> 
>     Hi Rei,
> 
>     I was thinking something along the lines of the attached patch to address this problem.  Could you try it out and see whether that resolves the issue?  Also do you have a reproducer that could be added to the testsuite to verify that this problem is fixed and doesn't get reintroduced into oprofile?
> 
>     -Will
> 
>