Re: [PATCH] elf: Change TLS static surplus default back to 1664

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 07/17/2020 11:53, Florian Weimer via Libc-alpha wrote:
> Make the computation in elf/dl-tls.c more transparent, and add
> an explicit test for the historic value.
> 
> Tested on powerpc64le-linux-gnu and x86_64-linux-gnu.

OK with one comment fix.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> index 9a17427047..462b0f46c1 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -54,13 +54,35 @@
>     Audit modules use their own namespaces, they are not included in rtld.nns,
>     but come on top when computing the number of namespaces.  */
>  
> -/* Size of initial-exec TLS in libc.so.  */
> -#define LIBC_IE_TLS 160
> +/* Size of initial-exec TLS in libc.so.  May be smaller than that on
> +   some architectures.  */
> +#define LIBC_IE_TLS 144

to me it's not clear what is smaller than what.
on some architectures libc.so has > 144 bytes
ie tls?

note that alignment is not considered in the
computation (i just assumed that if all sizes
are rounded up to 16bytes that works, if
something has larger tls alignment requirement
then that should be included into the size)
but i think 144 is OK.

> +
>  /* Size of initial-exec TLS in libraries other than libc.so.
>     This should be large enough to cover runtime libraries of the
>     compiler such as libgomp and libraries in libc other than libc.so.  */
>  #define OTHER_IE_TLS 144
>  
> +/* Default number of namespaces.  */
> +#define DEFAULT_NNS 4
> +
> +/* Default for dl_tls_static_optional.  */
> +#define OPTIONAL_TLS 512
> +
> +/* Compute the static TLS surplus based on the namespace count and the
> +   TLS space that can be used for optimizations.  */
> +static inline int
> +tls_static_surplus (int nns, int opt_tls)
> +{
> +  return (nns - 1) * LIBC_IE_TLS + nns * OTHER_IE_TLS + opt_tls;
> +}
> +
> +/* This value is chosen so that with default values for the tunables,
> +   the computation of dl_tls_static_surplus in
> +   _dl_tls_static_surplus_init yields the historic value 1664, for
> +   backwards compatibility.  */
> +#define LEGACY_TLS (1664 - tls_static_surplus (DEFAULT_NNS, OPTIONAL_TLS))

note that this is for the dynamic linking case.

with static linking the historical size was
2048 bytes static tls unconditionally, i
significantly reduced that with my patch
because then DL_NNS==1, this adds back a
bit of extra static tls in that case, but
that's OK.

> +
>  /* Calculate the size of the static TLS surplus, when the given
>     number of audit modules are loaded.  Must be called after the
>     number of audit modules is known and before static TLS allocation.  */
> @@ -74,8 +96,8 @@ _dl_tls_static_surplus_init (size_t naudit)
>    opt_tls = TUNABLE_GET (optional_static_tls, size_t, NULL);
>  #else
>    /* Default values of the tunables.  */
> -  nns = 4;
> -  opt_tls = 512;
> +  nns = DEFAULT_NNS;
> +  opt_tls = OPTIONAL_TLS;
>  #endif
>    if (nns > DL_NNS)
>      nns = DL_NNS;
> @@ -85,9 +107,8 @@ _dl_tls_static_surplus_init (size_t naudit)
>    nns += naudit;
>  
>    GL(dl_tls_static_optional) = opt_tls;
> -  GLRO(dl_tls_static_surplus) = ((nns - 1) * LIBC_IE_TLS
> -				 + nns * OTHER_IE_TLS
> -				 + opt_tls);
> +  assert (LEGACY_TLS >= 0);
> +  GLRO(dl_tls_static_surplus) = tls_static_surplus (nns, opt_tls) + LEGACY_TLS;
>  }

OK.