mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Colin Cross <ccross@google.com>
To: Rich Felker <dalias@libc.org>
Cc: musl@lists.openwall.com
Subject: Re: [musl] [PATCH] Add mallinfo2 and mallinfo
Date: Fri, 7 Jan 2022 11:41:49 -0800	[thread overview]
Message-ID: <CAMbhsRT5oSPQoX7vtuna8FcmfE07HQFk=YzoBAKytG5euoGhzg@mail.gmail.com> (raw)
In-Reply-To: <20220107033256.GJ7074@brightrain.aerifal.cx>

On Thu, Jan 6, 2022 at 7:32 PM Rich Felker <dalias@libc.org> wrote:
>
> On Thu, Jan 06, 2022 at 03:42:52PM -0800, Colin Cross wrote:
> > On Thu, Jan 6, 2022 at 2:00 PM Rich Felker <dalias@libc.org> wrote:
> > >
> > > On Thu, Jan 06, 2022 at 12:37:09PM -0800, Colin Cross wrote:
> > > > glibc introduced mallinfo2 [1], which solves some of the arguments [2]
> > > > against including mallinfo in musl by expanding the width of the
> > > > returned counters from int to size_t.
> > > >
> > > > This patch implements mallinfo2 without requiring any additional
> > > > metadata.  It iterates through the meta_areas and metas in order
> > > > to count mmap, large and small allocations, and produces ordblks,
> > > > hblks, hblkhd, uordblks and fordblks values.
> > > >
> > > > Once mallinfo2 exists, it is trivial to implement mallinfo that caps
> > > > the mallinfo2 outputs such that they fit in the int fields returned
> > > > by mallinfo.
> > > >
> > > > [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=e3960d1c57e57f33e0e846d615788f4ede73b945
> > > > [2] https://www.openwall.com/lists/musl/2018/01/17/2
> > >
> > > Historically, mallinfo was omitted intentionally in musl partly
> > > because of the wrong-types issue (fixed by mallinfo2), but also partly
> > > because the set of data items returned is built around certain
> > > assumptions about the malloc implementation that aren't necessarily
> > > valid, especially for our allocators. This could be revisited, but I'm
> > > not sure we'll find good justification to add it.
> >
> > Many of the mallinfo fields are meaningless and left zero.  I left
> > arenas and keepcost zero because mallocng never puts allocations on
> > the heap, only metadata.  Similarly, all the fastbin-related fields
> > are zero because fastbin seems very specific to glibc's
> > implementation.  The remainder seem to map to mallocng fairly well:
> > hblks and hblkhd track the number and total size of mmap allocations,
> > uordblks tracks the total size of all allocations, ordblks and
> > fordblks track the number and total size of the free slots.
>
> Here I'd tend to disagree and I think this highlights how the model
> doesn't map well. I would not call the metadata "the heap" just
> because it's (when possible) obtained with brk. I would call "the
> heap" the storage that's managed by the allocator's bookkeeping for
> reuse without going through a per-allocation syscall to allocate and
> free it, and distinguish that only from individually-mmapped units
> that are guaranteed to be unmapped when freed.

I think you're right, I'll fix arena to count the bytes in the grouped
allocations.

> > > > The motivation for this patch is an attempt to use musl instead of glibc
> > > > to build host tools used when building the Android platform and the
> > > > tools that are distributed to app developers as part of the Android SDK.
> > > > mallinfo is used in a variety of third-party code built as part of
> > > > building Android, and tests and benchmarks in the Android tree.
> > > >
> > > > The implementation has been lightly tested with bionic's malloc.mallinfo
> > > > and malloc.mallinfo2 tests, which verify that a variety of different
> > > > allocation sizes result in an increase of the uordblks value by at
> > > > least the usable size of the returned allocation.
> > > >
> > > > I can keep this as a local patch in Android if it is still not acceptable
> > > > for musl.
> > >
> > > Is there a reason not to just #ifdef HAVE_MALLINFO it out, or do a
> > > dummy implementation, or one that makes up semi-reasonable numbers
> > > purely based on /proc/self/maps without poking at malloc internals?
> >
> > It's often used in tests and benchmarks to verify that calling the
> > method repeatedly doesn't leak memory.  Given how easy it was to
> > implement I'd probably keep this implementation in Android rather than
> > try to deduce it from /proc/self/maps, which could contain many
> > non-malloc anonymous pages and can't tell the difference between
> > allocated and freed memory.
>
> For testing lack of memory leak, I'd think "whole process vm size"
> would be more meaningful than something from mallinfo. For example
> mallinfo wouldn't catch accidentally re-mmapping a file each time it's
> used without unmapping it, or manual mmap of MAP_ANON.
>
> > Somewhat relatedly, some patches I wrote for the kernel in Android to
> > allow naming anonymous memory regions have finally gone into the
> > linux-next branch [1].  If that becomes widely enabled then musl's
> > allocator could tag mmap regions, which would provide more information
> > for parsing stats out of /proc/self/maps.  It still couldn't tell the
> > difference between allocated and free though.
>
> I don't think this is something we'd do; it would significantly
> increase runtime cost. (Keep in mind mallocng's groups are not
> intended to necessarily be long-lived.)
>
> > > > diff --git a/src/malloc/mallocng/mallinfo.c b/src/malloc/mallocng/mallinfo.c
> > > > new file mode 100644
> > > > index 00000000..c60840b1
> > > > --- /dev/null
> > > > +++ b/src/malloc/mallocng/mallinfo.c
> > > > @@ -0,0 +1,73 @@
> > > > +#include <limits.h>
> > > > +#include <malloc.h>
> > > > +#include <stddef.h>
> > > > +
> > > > +#include "glue.h"
> > > > +#include "meta.h"
> > > > +
> > > > +static void accumulate_meta(struct mallinfo2 *mi, struct meta *g) {
> > > > +  int sc = g->sizeclass;
> > > > +  if (sc >= 48) {
> > > > +    // Large mmap allocation
> > > > +    mi->hblks++;
> > > > +    mi->uordblks += g->maplen*4096;
> > > > +    mi->hblkhd += g->maplen*4096;
> > > > +  } else {
> > > > +    if (g->freeable && !g->maplen) {
> > > > +      // Small size slots are embedded in a larger slot, avoid double counting
> > > > +      // by subtracing the size of the larger slot from the total used memory.
> > > > +      struct meta* outer_g = get_meta((void*)g->mem);
> > > > +      int outer_sc  = outer_g->sizeclass;
> > > > +      int outer_sz = size_classes[outer_sc]*UNIT;
> > > > +      mi->uordblks -= outer_sz;
> > > > +    }
> > > > +    int sz = size_classes[sc]*UNIT;
> > > > +    int mask = g->avail_mask | g->freed_mask;
> > > > +    int nr_unused = __builtin_popcount(mask);
> > > > +    mi->ordblks += nr_unused;
> > > > +    mi->uordblks += sz*(g->last_idx+1-nr_unused);
> > > > +    mi->fordblks += sz*nr_unused;
> > > > +  }
> > > > +}
> > >
> > > For upstreaming, __builtin_popcount wouldn't be usable. But aside from
> > > that, the approach here looks roughly correct. I don't see any
> > > correction for the case where a g->last_idx==1 and sc<48, in which
> > > case it's possible that map_len is less than the length for the size
> > > class. These should probably be treated like "individually mmapped"
> > > allocations. This is one place where trying to fit the mallinfo data
> > > model with an allocator that doesn't match its assumptions is
> > > something of a hack.
> >
> > I'll take a look at this, I assume you mean when g->last_idx==0?
>
> Yes. If g->last_idx==0, you need to use g->maplen to determine the
> size. Whether that's better counted as "individually mmapped" or
> "heap" I'm not sure.

Fixed in V2.  I counted it as "heap", since the mallinfo man page
explicitly mentions a size threshold for using mmap, and these are
below mallocng's mmap threshold.  I don't think it matters much which
way it is counted though.

> > > > +static void accumulate_meta_area(struct mallinfo2 *mi, struct meta_area *ma) {
> > > > +  for (int i=0; i<ma->nslots; i++) {
> > > > +    if (ma->slots[i].mem) {
> > > > +      accumulate_meta(mi, &ma->slots[i]);
> > > > +    }
> > > > +  }
> > > > +}
> > > > +
> > > > +struct mallinfo2 mallinfo2() {
> > > > +  struct mallinfo2 mi = {0};
> > > > +
> > > > +  rdlock();
> > > > +  struct meta_area *ma = ctx.meta_area_head;
> > > > +  while (ma) {
> > > > +    accumulate_meta_area(&mi, ma);
> > > > +    ma = ma->next;
> > > > +  }
> > > > +  unlock();
> > > > +
> > > > +  return mi;
> > > > +}
> > > > +
> > > > +#define cap(x) ((x > INT_MAX) ? INT_MAX : x)
> > > > +
> > > > +struct mallinfo mallinfo() {
> > > > +  struct mallinfo mi = {0};
> > > > +  struct mallinfo2 mi2 = mallinfo2();
> > > > +
> > > > +  mi.arena = cap(mi2.arena);
> > > > +  mi.ordblks = cap(mi2.ordblks);
> > > > +  mi.smblks = cap(mi2.smblks);
> > > > +  mi.hblks = cap(mi2.hblks);
> > > > +  mi.hblkhd = cap(mi2.hblkhd);
> > > > +  mi.usmblks = cap(mi2.usmblks);
> > > > +  mi.fsmblks = cap(mi2.fsmblks);
> > > > +  mi.uordblks = cap(mi2.uordblks);
> > > > +  mi.fordblks = cap(mi2.fordblks);
> > > > +  mi.keepcost = cap(mi2.keepcost);
> > > > +
> > > > +  return mi;
> > > > +}
> > > > --
> > > > 2.34.1.448.ga2b2bfdf31-goog
> > >
> > > If the API is added upstream, it really should be provided by both
> > > mallocng and oldmalloc, with the legacy mallinfo (int) wrapper, if
> > > any, in src/malloc rather than src/malloc/mallocng. Available
> > > functions should not differ based on --with-malloc choice.
> >
> > I can add the other implementations if this is likely to be accepted,
> > if I'm keeping this in Android I won't bother.
>
> Indeed, this is only relevant if it's adopted as public API.
>
> Rich

      reply	other threads:[~2022-01-07 19:42 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-06 20:37 Colin Cross
2022-01-06 22:00 ` Rich Felker
2022-01-06 23:42   ` Colin Cross
2022-01-07  3:32     ` Rich Felker
2022-01-07 19:41       ` Colin Cross [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMbhsRT5oSPQoX7vtuna8FcmfE07HQFk=YzoBAKytG5euoGhzg@mail.gmail.com' \
    --to=ccross@google.com \
    --cc=dalias@libc.org \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).