From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-10.9 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 2034 invoked from network); 7 Jan 2022 19:42:16 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 7 Jan 2022 19:42:16 -0000 Received: (qmail 3277 invoked by uid 550); 7 Jan 2022 19:42:14 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 3245 invoked from network); 7 Jan 2022 19:42:13 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=wfjefIRi0pfDuokczjTf/cfQIJLfibZ6AKFTEY2b/38=; b=hrLXHdfj3kuvr9FnMBFe2Ak0ZCU4dT+ej/xewQ2yog0vs7aVLPIe0TbGZ+/bIOQba4 2tJWwrL0PMWx4NDaf1n94htrTpt2hVTi16SY3cOM/TDiE83Y3oGfIDP4qGlz8T90jMmO SPDLxlNOhB9eWsSGz32O/SraZeoE5rudbdvGbHAPee+5FdOILCq1tO6v73UAuFSEOD3p FQWnPbZsLuLuhElUlTOwjSSIPc5Kx0BzTCFf9Q4VOWdsLfBhu38v9lktS6sL2hiWoUFb +WXEi18QoEsKWjzudC8HbnnMOvb1RJzmM2bdRksuwn3Du/8c00Ql8p4yXfmjO/ea23ul JR1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wfjefIRi0pfDuokczjTf/cfQIJLfibZ6AKFTEY2b/38=; b=ySfxH5qRC2A+emz+eZVqPVNve6r6dP4UxChkwtF0SheeHSBA1WZqtAUZcVS8eHsfW/ 6GujLtZVUp1+GHXCcG/VqKZUAGoaGqdycE+GwtDiu+6G6rADoWMpvEqQZ5Ku6CWps7qJ RrwFWPio8fHemsr1b/bvDTcDMpgp+tG5WQfZhtUQU4CaRlUTvjz8+EIahgzVmXwiCpp1 vrX6n6VVMeelsrqVuZ2gYPSpyn3BaR4dV9yuspNwMCvUmkdA6CDZfCtciy9YPupVhjuY QjzaPXc6A+KWxDrxI9WezPnB0jFym0wf2jJJHfc0jy2s8ZOTyzaG+bYNvTQSS559u88d FMYA== X-Gm-Message-State: AOAM531uy2I+lxctif5x8ZxOJ3OV2o7qFLHGO2CvddialJEc//Bri8du gTFdluK/J6C3RUICPJgkAtYGxy5IyElvryCsIKKTwA== X-Google-Smtp-Source: ABdhPJwTiIORzJonuhXbwSobaF7Rfx95XoBjuilSjJtdV+m7N44OCXSTS2LLMatT+y9L2lIMySBzs6asWGcQvLuuZUE= X-Received: by 2002:a25:2687:: with SMTP id m129mr59502936ybm.155.1641584521213; Fri, 07 Jan 2022 11:42:01 -0800 (PST) MIME-Version: 1.0 References: <20220106203709.1525763-1-ccross@google.com> <20220106220014.GI7074@brightrain.aerifal.cx> <20220107033256.GJ7074@brightrain.aerifal.cx> In-Reply-To: <20220107033256.GJ7074@brightrain.aerifal.cx> From: Colin Cross Date: Fri, 7 Jan 2022 11:41:49 -0800 Message-ID: To: Rich Felker Cc: musl@lists.openwall.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [musl] [PATCH] Add mallinfo2 and mallinfo On Thu, Jan 6, 2022 at 7:32 PM Rich Felker 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 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 > > > > +#include > > > > +#include > > > > + > > > > +#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; inslots; 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