Re: [patch] Updated fix for BZ#208: mallinfo returns incomplete information

[Andreas Jaeger <aj at suse dot com>]

On 05/15/2012 06:50 PM, Paul Pluzhnikov wrote:
> Greetings,
>
> As noted in http://sourceware.org/bugzilla/show_bug.cgi?id=208,
> mallinfo() only returns information about the memory used in the main arena,
> which is misleading at best.
>
> Attached is the patch I mailed earlier:
>    http://cygwin.com/ml/libc-alpha/2011-08/msg00116.html,
> rebased to current git trunk.
>
> Tested on Linux/x86_64 and Linux/i686.
>
> Please note: the patch violates coding style to match the rest of
> the file.  If converting to correct coding style is deemed desirable,
> I will send a separate patch to do that.

We haven't done this in the past and if we do it, we should IMO update 
the whole file. What do others think?



> Thanks,
> --
> Paul Pluzhnikov
>
>
> 2012-05-15  Paul Pluzhnikov<ppluzhnikov@google.com>
>
> 	[BZ #208]

wow, that's an ancient one.

> 	* malloc.c (int_mallinfo): Add parameter.

You change more than that. What about "Add parameter to collect 
statistics in it instead of returning it. Returns void." ?

> 	(__libc_mallinfo): Accumulate over all arenas.
> 	(__malloc_stats): Adjust.

I suggest to be a bit more verbose like "Adjusted for changed 
__int_mallinfo interface."

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 79cdc52..1dc739e 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -4515,10 +4515,9 @@ __malloc_usable_size(void* m)
>     ------------------------------ mallinfo ------------------------------
>   */
>
> -static struct mallinfo
> -int_mallinfo(mstate av)
> +static void
> +int_mallinfo(mstate av, struct mallinfo *m)

Could you add a comment to the function explaining its parameters, please?

The code itself looks fine to me, could you resend with the changes 
above, please?

thanks
Andreas

>   {
> -  struct mallinfo mi;
>     size_t i;
>     mbinptr b;
>     mchunkptr p;
> @@ -4558,29 +4557,40 @@ int_mallinfo(mstate av)
>       }
>     }
>
> -  mi.smblks = nfastblocks;
> -  mi.ordblks = nblocks;
> -  mi.fordblks = avail;
> -  mi.uordblks = av->system_mem - avail;
> -  mi.arena = av->system_mem;
> -  mi.hblks = mp_.n_mmaps;
> -  mi.hblkhd = mp_.mmapped_mem;
> -  mi.fsmblks = fastavail;
> -  mi.keepcost = chunksize(av->top);
> -  mi.usmblks = mp_.max_total_mem;
> -  return mi;
> +  m->smblks += nfastblocks;
> +  m->ordblks += nblocks;
> +  m->fordblks += avail;
> +  m->uordblks += av->system_mem - avail;
> +  m->arena += av->system_mem;
> +  m->fsmblks += fastavail;
> +  if (av ==&main_arena)
> +    {
> +      m->hblks = mp_.n_mmaps;
> +      m->hblkhd = mp_.mmapped_mem;
> +      m->usmblks = mp_.max_total_mem;
> +      m->keepcost = chunksize(av->top);
> +    }
>   }
>
>
>   struct mallinfo __libc_mallinfo()
>   {
>     struct mallinfo m;
> +  mstate ar_ptr;
>
>     if(__malloc_initialized<  0)
>       ptmalloc_init ();
> -  (void)mutex_lock(&main_arena.mutex);
> -  m = int_mallinfo(&main_arena);
> -  (void)mutex_unlock(&main_arena.mutex);
> +
> +  memset(&m, 0, sizeof (m));
> +  ar_ptr =&main_arena;
> +  do {
> +    (void)mutex_lock(&ar_ptr->mutex);
> +    int_mallinfo(ar_ptr,&m);
> +    (void)mutex_unlock(&ar_ptr->mutex);
> +
> +    ar_ptr = ar_ptr->next;
> +  } while (ar_ptr !=&main_arena);
> +
>     return m;
>   }
>
> @@ -4593,7 +4603,6 @@ __malloc_stats()
>   {
>     int i;
>     mstate ar_ptr;
> -  struct mallinfo mi;
>     unsigned int in_use_b = mp_.mmapped_mem, system_b = in_use_b;
>   #if THREAD_STATS
>     long stat_lock_direct = 0, stat_lock_loop = 0, stat_lock_wait = 0;
> @@ -4605,8 +4614,11 @@ __malloc_stats()
>     int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
>     ((_IO_FILE *) stderr)->_flags2 |= _IO_FLAGS2_NOTCANCEL;
>     for (i=0, ar_ptr =&main_arena;; i++) {
> +    struct mallinfo mi;
> +
> +    memset(&mi, 0, sizeof(mi));
>       (void)mutex_lock(&ar_ptr->mutex);
> -    mi = int_mallinfo(ar_ptr);
> +    int_mallinfo(ar_ptr,&mi);
>       fprintf(stderr, "Arena %d:\n", i);
>       fprintf(stderr, "system bytes     = %10u\n", (unsigned int)mi.arena);
>       fprintf(stderr, "in use bytes     = %10u\n", (unsigned int)mi.uordblks);


--
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126