Re: PING [patch] malloc: add mxfast tunable

[Carlos O'Donell <carlos at redhat dot com>]

On 8/8/19 3:24 PM, DJ Delorie wrote:
> https://www.sourceware.org/ml/libc-alpha/2018-11/msg00243.html
>
> There was much bikeshedding about the name of the tunable, but no actual
> patch review...

Reviewed. Please post v3. Three changes requested:
* Set mxfast to 0 in env var.
* Paragraph about intent of test.
* Clarify language in manual.

Thank you!

> DJ Delorie <dj@redhat.com> writes:
> > First pass at a tunable for MXFAST, which currently can only be
> > changed by a call to mallopt().
> >
> > 	* elf/dl-tunables.list: Add glibc.malloc.mxfast.
> > 	* manual/tunables.texi: Document it.
> > 	* malloc/malloc.c (do_set_mxfast): New.
> > 	(__libc_mallopt): Call it.
> > 	* malloc/arena.c: Add mxfast tunable.
> > 	* malloc/tst-mxfast.c: New.
> > 	* malloc/Makefile: Add it.
> >
> > diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> > index b108592b62..b7cc79f8bf 100644
> > --- a/elf/dl-tunables.list
> > +++ b/elf/dl-tunables.list
> > @@ -85,6 +85,11 @@ glibc {
> >       tcache_unsorted_limit {
> >         type: SIZE_T
> >       }
> > +    mxfast {
> > +      type: SIZE_T
> > +      minval: 0
> > +      security_level: SXID_IGNORE

OK, glibc.malloc.mxfast (MaXimum FAST bins size), and ignored by SETUID.

This name is historical and taken from the original mallopt implementations.

We want to keep this name for consistency.

We can always add a another tunable to change the name later after some
transitional period.

There is a maxval, but it's arch dependent e.g. (80 * SIZE_SZ / 4)

> > +    }
> >     }
> >     cpu {
> >       hwcap_mask {
> > diff --git a/malloc/Makefile b/malloc/Makefile
> > index 7d54bad866..c671edd646 100644
> > --- a/malloc/Makefile
> > +++ b/malloc/Makefile
> > @@ -38,6 +38,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
> >   	 tst-malloc_info \
> >   	 tst-malloc-too-large \
> >   	 tst-malloc-stats-cancellation \
> > +	 tst-mxfast \

OK, good a test.

> >   
> >   tests-static := \
> >   	 tst-interpose-static-nothread \
> > @@ -195,6 +196,8 @@ tst-malloc-usable-static-ENV = $(tst-malloc-usable-ENV)
> >   tst-malloc-usable-tunables-ENV = GLIBC_TUNABLES=glibc.malloc.check=3
> >   tst-malloc-usable-static-tunables-ENV = $(tst-malloc-usable-tunables-ENV)
> >   
> > +tst-mxfast-ENV = GLIBC_TUNABLES=glibc.malloc.tcache_count=0:glibc.malloc.mxfast=1

Why isn't this 0?

> > +
> >   ifeq ($(experimental-malloc),yes)
> >   CPPFLAGS-malloc.c += -DUSE_TCACHE=1
> >   else
> > diff --git a/malloc/arena.c b/malloc/arena.c
> > index 497ae475e7..2cad4344ea 100644
> > --- a/malloc/arena.c
> > +++ b/malloc/arena.c
> > @@ -237,6 +237,7 @@ TUNABLE_CALLBACK_FNDECL (set_tcache_max, size_t)
> >   TUNABLE_CALLBACK_FNDECL (set_tcache_count, size_t)
> >   TUNABLE_CALLBACK_FNDECL (set_tcache_unsorted_limit, size_t)
> >   #endif
> > +TUNABLE_CALLBACK_FNDECL (set_mxfast, size_t)

OK. Enable the tunable.

> >   #else
> >   /* Initialization routine. */
> >   #include <string.h>
> > @@ -324,6 +325,7 @@ ptmalloc_init (void)
> >     TUNABLE_GET (tcache_unsorted_limit, size_t,
> >   	       TUNABLE_CALLBACK (set_tcache_unsorted_limit));
> >   # endif
> > +  TUNABLE_GET (mxfast, size_t, TUNABLE_CALLBACK (set_mxfast));

OK, get the value during ptmalloc_init.

> >   #else
> >     const char *s = NULL;
> >     if (__glibc_likely (_environ != NULL))
> > diff --git a/malloc/malloc.c b/malloc/malloc.c
> > index 67cdfd0ad2..f6c2a18f7d 100644
> > --- a/malloc/malloc.c
> > +++ b/malloc/malloc.c
> > @@ -5137,6 +5137,19 @@ do_set_tcache_unsorted_limit (size_t value)
> >   }
> >   #endif
> >   
> > +static inline int
> > +__always_inline
> > +do_set_mxfast (size_t value)
> > +{
> > +  if (value >= 0 && value <= MAX_FAST_SIZE)

OK, limit the value.

> > +    {
> > +      LIBC_PROBE (memory_mallopt_mxfast, 2, value, get_max_fast ());
> > +      set_max_fast (value);
> > +      return 1;
> > +    }
> > +  return 0;
> > +}

OK, refactor code from mallopt to here.

> > +
> >   int
> >   __libc_mallopt (int param_number, int value)
> >   {
> > @@ -5156,13 +5169,7 @@ __libc_mallopt (int param_number, int value)
> >     switch (param_number)
> >       {
> >       case M_MXFAST:
> > -      if (value >= 0 && value <= MAX_FAST_SIZE)
> > -        {
> > -          LIBC_PROBE (memory_mallopt_mxfast, 2, value, get_max_fast ());
> > -          set_max_fast (value);
> > -        }
> > -      else
> > -        res = 0;
> > +      do_set_mxfast (value);

OK.

> >         break;
> >   
> >       case M_TRIM_THRESHOLD:
> > diff --git a/malloc/tst-mxfast.c b/malloc/tst-mxfast.c
> > new file mode 100644
> > index 0000000000..306f748736
> > --- /dev/null
> > +++ b/malloc/tst-mxfast.c
> > @@ -0,0 +1,46 @@
> > +/* Test that glibc.malloc.mxfast tunable works.
> > +   Copyright (C) 2018 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +#include <malloc.h>
> > +#include <assert.h>
> > +

Test needs a paragraph explaining the intent of the test.

> > +int
> > +do_test(void)
> > +{
> > +  struct mallinfo m;
> > +  char * volatile p1;
> > +  char * volatile p2;
> > +
> > +  /* Arbitrary value; must be in default fastbin range.  */
> > +  p1 = malloc (3);
> > +  /* Something large so that p1 isn't a "top block" */
> > +  p2 = malloc (512);
> > +  free (p1);
> > +
> > +  m = mallinfo();
> > +
> > +  /* This will fail if there are any blocks in the fastbins.  */
> > +  assert (m.smblks == 0);

OK.

> > +
> > +  /* To keep gcc happy.  */
> > +  free (p2);
> > +
> > +  return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
> > diff --git a/manual/tunables.texi b/manual/tunables.texi
> > index 3345a23969..27593c9469 100644
> > --- a/manual/tunables.texi
> > +++ b/manual/tunables.texi
> > @@ -213,6 +213,18 @@ pre-fill the per-thread cache with.  The default, or when set to zero,
> >   is no limit.
> >   @end deftp
> >   
> > +@deftp Tunable glibc.malloc.mxfast
> > +One of the optimizations malloc uses is to maintain a series of ''fast
> > +bins'' that hold chunks up to a specific size.  The default and
> > +maximum size which may be held this way is 80 bytes on 32-bit systems
> > +or 160 bytes on 64-bit systems.  Applications which value size over
> > +speed may choose to reduce the number of fast bins with this tunable.

"may choose to reduce the size of requests which are serviced from fast bins..."

We don't control the number, we just control the size.

> > +Note that the value specified includes malloc's internal overhead,
> > +which is normally the size of one pointer, so add 4 on 32-bit systems
> > +or 8 on 64-bit systems to the size passed to @code{malloc} for the
> > +largest bin size to enable.

That sucks, but it is what it is.

This is why we should deprecate this in favour of a tunable that doesn't
force you to do math like this, it should just be about memory sizes from
a user perspective.

> > +@end deftp
> > +
> >   @node Elision Tunables
> >   @section Elision Tunables
> >   @cindex elision tunables


--
Cheers,
Carlos.