Ok I applied your suggestions and move back the malloc_trim/mallinfo part as it was before.
On Thu, 25 Nov 2021 at 14:37, Willy Tarreau <[email protected]> wrote: > > On Thu, Nov 25, 2021 at 01:19:39PM +0000, David CARLIER wrote: > > Here a patchset instead :) > > Thanks! > > I've reviewed it, I'm having some comments below: > > > From e8daa477b53a43ab39113cf0e9c43d9bbda1e9a9 Mon Sep 17 00:00:00 2001 > > From: David Carlier <[email protected]> > > Date: Thu, 25 Nov 2021 10:26:50 +0000 > > Subject: [PATCH 1/3] MINOR: pool: refactoring to make it available for other > > systems/allocators. > > > > Actually it's focusing on linux/glibc, we re trying to expand it for other > > possible systems and allocators combinations. > > --- > > include/haproxy/compat.h | 9 +++++++-- > > src/pool.c | 38 +++++++------------------------------- > > 2 files changed, 14 insertions(+), 33 deletions(-) > > > > diff --git a/include/haproxy/compat.h b/include/haproxy/compat.h > > index 25b15a1f0..b801dac08 100644 > > --- a/include/haproxy/compat.h > > +++ b/include/haproxy/compat.h > > @@ -263,9 +263,14 @@ typedef struct { } empty_t; > > #endif > > > > /* glibc 2.33 provides mallinfo2() that overcomes mallinfo()'s type > > limitations */ > > -#if (defined(__GNU_LIBRARY__) && (__GLIBC__ > 2 || __GLIBC__ == 2 && > > __GLIBC_MINOR__ >= 33)) > > +#if defined(__GNU_LIBRARY__) > > #include <malloc.h> > > -#define HA_HAVE_MALLINFO2 > > +#if (__GLIBC__ > 2 || __GLIBC__ == 2 && __GLIBC_MINOR__ >= 33) > > +#define HA_MALLINFO_API mallinfo2 > > +#else > > +#define HA_MALLINFO_API mallinfo > > +#endif > > +#define HA_HAVE_MALLINFO > > #endif > > > > /* FreeBSD also has malloc_usable_size() but it requires malloc_np.h */ > > diff --git a/src/pool.c b/src/pool.c > > index af46b4469..16ef76cf0 100644 > > --- a/src/pool.c > > +++ b/src/pool.c > > @@ -42,14 +42,15 @@ int mem_poison_byte = -1; > > static int mem_fail_rate = 0; > > #endif > > > > -#if defined(HA_HAVE_MALLOC_TRIM) > > static int using_libc_allocator = 0; > > > > /* ask the allocator to trim memory pools */ > > static void trim_all_pools(void) > > { > > +#ifdef HAVE_MALLOC_TRIM > > if (using_libc_allocator) > > malloc_trim(0); > > +#endif > > } > > > > /* check if we're using the same allocator as the one that provides > > @@ -60,48 +61,23 @@ static void trim_all_pools(void) > > */ > > static void detect_allocator(void) > > { > > -#ifdef HA_HAVE_MALLINFO2 > > - struct mallinfo2 mi1, mi2; > > -#else > > - struct mallinfo mi1, mi2; > > -#endif > > +#if defined(HA_HAVE_MALLINFO) > > + struct HA_MALLINFO_API mi1, mi2; > > void *ptr; > > > > -#ifdef HA_HAVE_MALLINFO2 > > - mi1 = mallinfo2(); > > -#else > > - mi1 = mallinfo(); > > -#endif > > + mi1 = HA_MALLINFO_API(); > > ptr = DISGUISE(malloc(1)); > > -#ifdef HA_HAVE_MALLINFO2 > > - mi2 = mallinfo2(); > > -#else > > - mi2 = mallinfo(); > > -#endif > > + mi2 = HA_MALLINFO_API(); > > free(DISGUISE(ptr)); > > > > using_libc_allocator = !!memcmp(&mi1, &mi2, sizeof(mi1)); > > +#endif > > } > > I find it unwelcome to use a single macro for the struct name and > the function. It just turns out that they are the same but they're > for different object types, and if one day we have mallinfo3() that > works on struct mallinfo2, we're doomed. I hope it will never come to such things, sounds terrible to me :) even tough I have hard time to imagine a situation like this. And the gain in legibility > is not that great overall, I'd rather keep them with the ugly ifdefs > here. If you prefer, just write two blocks in the function, one for > mallinfo2 and one for the other. That's what I did the first time > but wasn't much convinced about the benefit, but at this point it's > a matter of taste. > > If you really want to have it this way, otherwise use two different > macros, e.g.: > #define HA_MALLINFO_TYPE struct mallinfo > #define HA_MALLINFO_FUNC(x) mallinfo(x) > > But again this adds abstraction that doesn't help much, especially in > tricky code which is written to diagnose certain things. > > > From c0f5cf6f935392e1ea69a84aae104de5ed06e68c Mon Sep 17 00:00:00 2001 > > From: David Carlier <[email protected]> > > Date: Thu, 25 Nov 2021 13:00:54 +0000 > > Subject: [PATCH 2/3] MEDIUM: pool : detecting jemalloc usage at runtime. > > > > Here we're just looking up if the mallctl's jemalloc API symbol is present. > > --- > > src/pool.c | 26 +++++++++++++++++++------- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/src/pool.c b/src/pool.c > > index 16ef76cf0..65c037911 100644 > > --- a/src/pool.c > > +++ b/src/pool.c > > @@ -43,6 +43,7 @@ static int mem_fail_rate = 0; > > #endif > > > > static int using_libc_allocator = 0; > > +static int(*my_mallctl)(const char *, void *, size_t *, void *, size_t) = > > NULL; > > > > /* ask the allocator to trim memory pools */ > > static void trim_all_pools(void) > > @@ -61,17 +62,28 @@ static void trim_all_pools(void) > > */ > > static void detect_allocator(void) > > { > > + extern int mallctl(const char *, void *, size_t *, void *, size_t) > > __attribute__((weak)); > > + > > + my_mallctl = mallctl; > > + > > + if (!my_mallctl) > > + my_mallctl = get_sym_curr_addr("mallctl"); > > + > > + if (!my_mallctl) { > > #if defined(HA_HAVE_MALLINFO) > > - struct HA_MALLINFO_API mi1, mi2; > > - void *ptr; > > + struct HA_MALLINFO_API mi1, mi2; > > + void *ptr; > > > > - mi1 = HA_MALLINFO_API(); > > - ptr = DISGUISE(malloc(1)); > > - mi2 = HA_MALLINFO_API(); > > - free(DISGUISE(ptr)); > > + mi1 = HA_MALLINFO_API(); > > + ptr = DISGUISE(malloc(1)); > > + mi2 = HA_MALLINFO_API(); > > + free(DISGUISE(ptr)); > > > > - using_libc_allocator = !!memcmp(&mi1, &mi2, sizeof(mi1)); > > + using_libc_allocator = !!memcmp(&mi1, &mi2, sizeof(mi1)); > > #endif > > + } else { > > + using_libc_allocator = 1; > > + } > > This "else" block will result in having "using_libc_allocator" set when > linking jemalloc with LD_PRELOAD, which is exactly what we tried to > avoid as it becomes misleading here. I understand why you did it this > way, though, it's a matter of definition of this variable's role. I > think we should stick to "the native libc's allocator is being used, > it was not changed at runtime using LD_PRELOAD". Then I think we could > rename it to "using_default_allocator", suggesting that all calls to > malloc/free pass via the same allocator from beginning to end, because > that was the problem we tried to address with it. Or maybe we invert > the logic and call it "using_external_allocator". > > In any case, the logic should be reversed: by default it is the same > allocator (thus we initialize it to 1). If mallctl is not defined but > it is found using dlsym() then we have the proof that we're facing an > external allocator, so we reset it. If mallctl is still not defined, > then fall back to the malloc_info test to set it. > > The simplest change for this would consist in setting it to 1 if > mallctl is non-null on the first test only, but I think that for the > long term this would remain misleading to do that only, hence the > better choice of making it reflect the internal-vs-external status. > > > From 7206aff8ce6b6cb8c9b9b36d33b4f218f33cf0d1 Mon Sep 17 00:00:00 2001 > > From: David Carlier <[email protected]> > > Date: Thu, 25 Nov 2021 13:14:37 +0000 > > Subject: [PATCH 3/3] MEDIUM: pool: using jemalloc when relevant. > > > > Finally, we trim the arenas with jemalloc if detected otherwise falling back > > to glibc's malloc_trim. > > --- > > src/pool.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/src/pool.c b/src/pool.c > > index 65c037911..3ec7a5386 100644 > > --- a/src/pool.c > > +++ b/src/pool.c > > @@ -48,10 +48,24 @@ static int(*my_mallctl)(const char *, void *, size_t *, > > void *, size_t) = NULL; > > /* ask the allocator to trim memory pools */ > > static void trim_all_pools(void) > > { > > + if (using_libc_allocator) { > > + if (my_mallctl) { > > Here that results in testing directly my_mallctl() without checking > using_libc_allocator. > > > + unsigned int i, narenas = 0; > > + size_t len = sizeof(narenas); > > + > > + if (my_mallctl("arenas.narenas", &narenas, &len, > > NULL, 0) == 0) { > > + for (i = 0; i < narenas; i ++) { > > + char mib[32] = {0}; > > + snprintf(mib, sizeof(mib), > > "arena.%u.purge", i); > > + (void)my_mallctl(mib, NULL, NULL, > > NULL, 0); > > + } > > + } > > + } else { > > And moving the test here. > > > #ifdef HAVE_MALLOC_TRIM > > - if (using_libc_allocator) > > - malloc_trim(0); > > + malloc_trim(0); > > #endif > > + } > > + } > > } > > Willy
From 776c50989ed63d851f358a3cf931dab5287d1e89 Mon Sep 17 00:00:00 2001 From: David Carlier <[email protected]> Date: Thu, 25 Nov 2021 16:14:38 +0000 Subject: [PATCH 2/2] MEDIUM: pool using jemalloc to purge arenas for trim_all_pools. In the case of Linux/glibc, falling back to malloc_trim if jemalloc had not been detected beforehand. --- src/pool.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/pool.c b/src/pool.c index ab995b10a..506e275d4 100644 --- a/src/pool.c +++ b/src/pool.c @@ -48,10 +48,23 @@ static int(*my_mallctl)(const char *, void *, size_t *, void *, size_t) = NULL; /* ask the allocator to trim memory pools */ static void trim_all_pools(void) { + if (my_mallctl) { + unsigned int i, narenas = 0; + size_t len = sizeof(narenas); + + if (my_mallctl("arenas.narenas", &narenas, &len, NULL, 0) == 0) { + for (i = 0; i < narenas; i ++) { + char mib[32] = {0}; + snprintf(mib, sizeof(mib), "arena.%u.purge", i); + (void)my_mallctl(mib, NULL, NULL, NULL, 0); + } + } + } else { #if defined(HA_HAVE_MALLOC_TRIM) - if (using_default_allocator) - malloc_trim(0); + if (using_default_allocator) + malloc_trim(0); #endif + } } /* check if we're using the same allocator as the one that provides -- 2.33.1
From 5942f768dd263403e3af3183d6a364b237320255 Mon Sep 17 00:00:00 2001 From: David Carlier <[email protected]> Date: Thu, 25 Nov 2021 16:09:45 +0000 Subject: [PATCH 1/2] MEDIUM: pool refactor malloc_trim/glibc and jemalloc api addition detections. Attempts to detect jemalloc at runtime before hand whether linked or via symbols overrides. Falling back to malloc_trim/glibc for Linux otherwise. --- src/pool.c | 58 +++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/pool.c b/src/pool.c index af46b4469..ab995b10a 100644 --- a/src/pool.c +++ b/src/pool.c @@ -42,14 +42,16 @@ int mem_poison_byte = -1; static int mem_fail_rate = 0; #endif -#if defined(HA_HAVE_MALLOC_TRIM) -static int using_libc_allocator = 0; +static int using_default_allocator = 1; +static int(*my_mallctl)(const char *, void *, size_t *, void *, size_t) = NULL; /* ask the allocator to trim memory pools */ static void trim_all_pools(void) { - if (using_libc_allocator) +#if defined(HA_HAVE_MALLOC_TRIM) + if (using_default_allocator) malloc_trim(0); +#endif } /* check if we're using the same allocator as the one that provides @@ -60,48 +62,46 @@ static void trim_all_pools(void) */ static void detect_allocator(void) { + extern int mallctl(const char *, void *, size_t *, void *, size_t) __attribute__((weak)); + + my_mallctl = mallctl; + + if (!my_mallctl) { + my_mallctl = get_sym_curr_addr("mallctl"); + using_default_allocator = (my_mallctl == NULL); + } + + if (!my_mallctl) { +#if defined(HA_HAVE_MALLOC_TRIM) #ifdef HA_HAVE_MALLINFO2 - struct mallinfo2 mi1, mi2; + struct mallinfo2 mi1, mi2; #else - struct mallinfo mi1, mi2; + struct mallinfo mi1, mi2; #endif - void *ptr; + void *ptr; #ifdef HA_HAVE_MALLINFO2 - mi1 = mallinfo2(); + mi1 = mallinfo2(); #else - mi1 = mallinfo(); + mi1 = mallinfo(); #endif - ptr = DISGUISE(malloc(1)); + ptr = DISGUISE(malloc(1)); #ifdef HA_HAVE_MALLINFO2 - mi2 = mallinfo2(); + mi2 = mallinfo2(); #else - mi2 = mallinfo(); + mi2 = mallinfo(); #endif - free(DISGUISE(ptr)); - - using_libc_allocator = !!memcmp(&mi1, &mi2, sizeof(mi1)); -} + free(DISGUISE(ptr)); -static int is_trim_enabled(void) -{ - return using_libc_allocator; -} -#else - -static void trim_all_pools(void) -{ -} - -static void detect_allocator(void) -{ + using_default_allocator = !!memcmp(&mi1, &mi2, sizeof(mi1)); +#endif + } } static int is_trim_enabled(void) { - return 0; + return using_default_allocator; } -#endif /* Try to find an existing shared pool with the same characteristics and * returns it, otherwise creates this one. NULL is returned if no memory -- 2.33.1
