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

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.

> 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?

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


Thanks,
Rei Odaira


2015-06-17 8:55 GMT-05:00 William Cohen <[email protected]>:

> 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 大平怜 <[email protected] <mailto:
> [email protected]>>:
> >>
> >>     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 <[email protected]>  2012-03-27 19:03:45
> > Committer: Maynard Johnson <[email protected]>  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
>
>

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o

_______________________________________________
oprofile-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/oprofile-list