Re: malloc probes for Systemtap

[Jeff Law <law at redhat dot com>]

On 09/04/2012 05:39 PM, Alexandre Oliva wrote:
> I was asked to introduce probes in malloc to aid in debugging cases in
> which our memory allocation code performs poorly.
>
> I've grouped the probes in separate patches, in which the probes are
> logically connected.  I'd appreciate feedback on the soundness of the
> probe set: did I miss anything in these groups that is worth probing?
> is any of the probes uselessly redundant?  are they desirable at all?
>
> Are they ok for master?  Do the docs look reasonable, description and
> formatting-wise?  (The only other probe docs I found in glibc were in
> elf/rtld-debugger-interface.txt)
>
> The version of the patchset I pushed to lxoliva/malloc-probes-bz742038
> depends on Siddhesh's patch that introduces get_another_arena.  Although
> I've just suggested changes to it, I decided to post the patchset as it
> is, since most of it applies cleanly anyway, and regardless of
> adjustments needed for Siddhesh's patch, I suspect we might go through
> more than one round of review before my patchset can go in anyway.
>
>
>
> 04-malloc-probes.patch
>
>
> Add first set of malloc probes.
>
> From: Alexandre Oliva<aoliva@redhat.com>
>
> for ChangeLog
> from  Alexandre Oliva<aoliva@redhat.com>
>
> 	* malloc/malloc.c: Include stap-probe.h.
> 	(__libc_mallopt): Add mallopt probe.
> 	* malloc/arena.c (_int_new_arena): Add malloc_arena_new probe.
> 	* manual/malloc.texi: Document them.
The probe for arena creation looks good.  A developer can infer a few 
useful things from hitting that probe.

Isn't the mallopt probe superseded by the second patch which adds more 
detailed probes (patch #2 gives the old/new values for everything, 
right) whereas this probe just gives the parameter and its requested new 
value.  I guess that might be useful if the new value is out of range 
and the caller didn't check mallopt's return value.




> Add probes for all changes to malloc options.
>
> From: Alexandre Oliva<aoliva@redhat.com>
>
> for ChangeLog
> from  Alexandre Oliva<aoliva@redhat.com>
>
> 	* malloc/malloc.c (__libc_free): Add
> 	mallopt_free_dyn_thresholds probe.
> 	(__libc_mallopt): Add multiple mallopt probes.
> 	* manual/malloc.texi: Document them.
This looks good as well.  One of the things we consistently see is 
confusion and questions around glibc's dynamic thresholds.  The ability 
to see how the mmap & trim thresholds are changing is definitely useful, 
particularly for longer running server applications.

Knowing that something is calling mallopt is a bit more subtle.  ie, for 
a give app, you'd think the app's developer would know if it calls 
mallopt.  However, with the number, size and complexity of libraries 
that apps link against, it's plausible that a library not under the 
developer's control could be calling mallopt and setting parameters to 
values that aren't particularly well suited for the application.


> section Statistics for Memory Allocation with @code{malloc}
>
>
>
> 06-malloc-probe-arena-changes.patch
>
>
> Add probes for malloc arena changes.
>
> From: Alexandre Oliva<aoliva@redhat.com>
>
> for ChangeLog
> from  Alexandre Oliva<aoliva@redhat.com>
>
> 	* malloc/arena.c (get_free_list): Add probe
> 	malloc_arena_reuse_free_list.
> 	(reused_arena) [PER_THREAD]: Add probes malloc_arena_reuse_wait
> 	and malloc_arena_reuse.
> 	(arena_get2) [!PER_THREAD]: Likewise.
> 	* malloc/malloc.c (__libc_realloc) [!PER_THREAD]: Add probe
> 	malloc_arena_reuse_realloc.
> 	* manual/malloc.texi: Document them.
This looks good.  Basically what we're trying to capture here is when a 
thread stops allocating from one area and starts allocating in a 
different arena.  This can indicate a few things which a developer might 
want to know (say for example a too low ARENA_MAX leading to contention 
on the arena locks).

The probes can also be used to see how long we block waiting for an 
arena to become available.


> 07-malloc-probe-retries.patch
>
>
> Add probes for malloc retries.
>
> From: Alexandre Oliva<aoliva@redhat.com>
>
> for ChangeLog
> from  Alexandre Oliva<aoliva@redhat.com>
>
> 	* malloc/malloc.c (__libc_malloc): Add malloc_retry probe.
> 	(__libc_realloc): Add realloc_retry probe.
> 	(__libc_memalign): Add memalign_retry probe.
> 	(__libc_valloc): Add valloc_retry probe.
> 	(__libc_pvalloc): Add pvalloc_retry probe.
> 	(__libc_calloc): Add calloc_retry probe.
> 	* manual/malloc.texi: Document them.
This looks good as well.  Basically we're just trying to track when 
allocation from the current arena fails and we try to get memory in a 
different arena.  This shouldn't happen often, but if it does it can be 
a sign of problems in the application.  Definitely useful.

> 09-malloc-probe-heaps.patch
>
>
> Add malloc probes for sbrk and heap resizing.
>
> From: Alexandre Oliva<aoliva@redhat.com>
>
> for ChangeLog
> from  Alexandre Oliva<aoliva@redhat.com>
>
> 	* malloc/arena.c (new_heap): New heap_new probe.
> 	(grow_heap): New heap_more probe.
> 	(shrink_heap): New heap_less probe.
> 	(heap_trim): New heap_free probe.
> 	* malloc/malloc.c (sysmalloc): New sbrk_more probe.
> 	(systrim): New sbrk_less probe.
> 	* manual/malloc.texi: Document them.
These are fairly low level probes which allow the programmer to monitor 
behaviour of allocations/deallocations using sbrk.  It might be useful 
in tracking down heap fragmentation and similar issues.

This looks good to me as well.

So there's the question of how to handle the documentation, which you 
posted a separate patch to handle.  Keeping the .texi bits but not 
including the probes in the formatted manual seems fine until we sort 
out the ABI/API issues around probes.

As far as the specific probes, the only on that seems iffy to me is the 
probe at the beginning of mallopt -- we can get all the same information 
from later probes, with the exception of values which are out of range. 
 But even that could easily be useful.

I think the whole set of probes ought to go in with your doc patch 
follow-up as well.

Jeff