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. 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
