Re: [PATCH] Fix tcache count maximum

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

On 5/7/19 10:30 AM, Wilco Dijkstra wrote:
> Hi Carlos,
>
> > Please create a bug for this.
> >
> > This is a publicly visible issue with tcache and tunables.
>
> Sure, BZ 24531.

Thanks.

> > This patch conflates two issues.
> >
> > (a) Adding checking to tunable.
> >
> > (b) Lifting limit.
> >
> > Please split into two bugs. One to fix tunables, another to raise the
> > tcache chunk caching limit.
>
> > If you are going to do (b) and change the sign of counts then you need
> > to go through and fix other code that expects to possibly have a
> > negative value.
>
> If there is any code that expects it to be negative that's a serious bug...
> Char is neither signed nor unsigned, the valid range for char is 0..127.

This is not correct.

Char's sign is implementation defined.

So it's not a serious bug, but it's a non-portable assumption we should fix.

I don't know if gcc makes the sign of char vary by architecture or not.

> 2939   ++(tcache->counts[tc_idx]);
>
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>          assert (tcache->counts[tc_idx] != 0);
>          See below for discussion.
>
> In all cases we already check tcache->counts[tc_idx] < mp_.tcache_count,
> so there can be no overflow iff mp_.tcache_count is the maximum value of
> tcache->counts[] entries. So no checks needed.
>
> 2947   tcache_entry *e = tcache->entries[tc_idx];
> 2948   assert (tc_idx < TCACHE_MAX_BINS);
> 2949   assert (tcache->counts[tc_idx] > 0);
>
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>          Always true now if counts is only positive.
>          Remove?
>
> Yes this assert is redundant since we already checked there is a valid entry
> (or just added several new entries). So this assert can never trigger, it only
> fails if tcache_put has an overflow bug.
>
> 2950   tcache->entries[tc_idx] = e->next;
> 2951   --(tcache->counts[tc_idx]);
>
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>          May wrap on error, should we check that and assert?
>          We expect the caller to check for != NULL entry,
>          indicating at least one entry. It's possible the list
>          is corrupt and 'e' is pointing to garbage, so an
>          assert might be good here?
>
>          assert (tcache->counts[tc_idx] != MAX_TCACHE_COUNT);
>
> No this can't underflow after we fix the overflow bug.

OK.

> > The manual/memory.texi needs updating because you made the
> > count twice the size, and the rough estimates for size of
> > tcache should be updated. The manual should also list the
> > actual limit being imposed.
>
> Which size do you mean? I can't find anything in manual/memory.texi
> refering to tcache. I did update the tunables which incorrectly states
> there is no limit on glibc.malloc.tcache_count.

When you extend the counts will you need to update the size estimates?

glibc/manual/tunables.texi:

195 The approximate maximum overhead of the per-thread cache is thus equal
196 to the number of bins times the chunk count in each bin times the size
197 of each chunk.  With defaults, the approximate maximum overhead of the
198 per-thread cache is approximately 236 KB on 64-bit systems and 118 KB
199 on 32-bit systems.
200 @end deftp


> See updated patch below - this should be simple and safe to backport.
>
> Cheers,
> Wilco
>
>
> [PATCH v2] Fix tcache count maximum (BZ #24531)
>
> The tcache counts[] array is a char, which has a very small range and thus
> may overflow.  When setting tcache_count tunable, there is no overflow check.
> However the tunable must not be larger than the maximum value of the tcache
> counts[] array, otherwise it can overflow when filling the tcache.
>
> Eg. export GLIBC_TUNABLES=glibc.malloc.tcache_count=4096
> leads to crashes in benchtests:
>
> Running /build/glibc/benchtests/bench-strcoll
> bench-strcoll: malloc.c:2949: tcache_get: Assertion `tcache->counts[tc_idx] > 0' failed.
> Aborted
>
> ChangeLog:
> 2019-05-07  Wilco Dijkstra  <wdijkstr@arm.com>
>
>          [BZ #24531]
>          * malloc/malloc.c (MAX_TCACHE_COUNT): New define.
>          (tcache_put): Remove redundant assert.
>          (do_set_tcache_count): Only update if count is small enough.
>          * manual/tunables.texi (glibc.malloc.tcache_count): Document max value.

This looks good to me!

Thank you.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 0e3d4dd5163f5fa8fb07b71fb7e318e7b10f5cfd..e03a14aabe5d4a1ca28eb5c0865e03606a70e1d6 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2905,6 +2905,8 @@ typedef struct tcache_perthread_struct
>     tcache_entry *entries[TCACHE_MAX_BINS];
>   } tcache_perthread_struct;
>   
> +#define MAX_TCACHE_COUNT 127	/* Maximum value of counts[] entries.  */
> +

OK.

>   static __thread bool tcache_shutting_down = false;
>   static __thread tcache_perthread_struct *tcache = NULL;
>   
> @@ -2932,7 +2934,6 @@ tcache_get (size_t tc_idx)
>   {
>     tcache_entry *e = tcache->entries[tc_idx];
>     assert (tc_idx < TCACHE_MAX_BINS);
> -  assert (tcache->counts[tc_idx] > 0);

OK.

>     tcache->entries[tc_idx] = e->next;
>     --(tcache->counts[tc_idx]);
>     e->key = NULL;
> @@ -5098,8 +5099,11 @@ do_set_tcache_max (size_t value)
>   static __always_inline int
>   do_set_tcache_count (size_t value)
>   {
> -  LIBC_PROBE (memory_tunable_tcache_count, 2, value, mp_.tcache_count);
> -  mp_.tcache_count = value;
> +  if (value <= MAX_TCACHE_COUNT)
> +    {
> +      LIBC_PROBE (memory_tunable_tcache_count, 2, value, mp_.tcache_count);
> +      mp_.tcache_count = value;
> +    }

OK.

>     return 1;
>   }
>   
> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index 749cabff1b003f20e36f793a268f5f77944aafbb..ae638823a21b9cc7aca3684c8e3067cb8cd287e0 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -189,8 +189,8 @@ per-thread cache.  The default (and maximum) value is 1032 bytes on
>   
>   @deftp Tunable glibc.malloc.tcache_count
>   The maximum number of chunks of each size to cache. The default is 7.
> -There is no upper limit, other than available system memory.  If set
> -to zero, the per-thread cache is effectively disabled.
> +The upper limit is 127.  If set to zero, the per-thread cache is effectively
> +disabled.

OK.

>   
>   The approximate maximum overhead of the per-thread cache is thus equal
>   to the number of bins times the chunk count in each bin times the size


--
Cheers,
Carlos.