[v2,1/1] rtld: add glibc.rtld.optional_static_tls_alignment

  Add an alignment tunable to go along with glibc.rtld.optional_static_tls
to optimize loading audit modules with higher-than-default alignment
requirements.  Unlike the extra size, this is only used when creating
the TLS area so need not be stored in GLRO().
---

changes in v2:
  - add the other tls case type for adding extra alignment

 elf/Makefile                 |  8 ++++++++
 elf/dl-tls.c                 |  9 +++++++++
 elf/dl-tunables.list         |  5 +++++
 elf/tst-tlsalign-tunable-a.c | 32 +++++++++++++++++++++++++++++
 elf/tst-tlsalign-tunable.c   | 39 ++++++++++++++++++++++++++++++++++++
 manual/tunables.texi         |  8 ++++++++
 6 files changed, 101 insertions(+)
 create mode 100644 elf/tst-tlsalign-tunable-a.c
 create mode 100644 elf/tst-tlsalign-tunable.c
  

* DJ Delorie:

>  ifeq (no,$(build-hardcoded-path-in-tests))
> @@ -1084,6 +1085,7 @@ modules-names += \
>    tst-tls22-mod2-gnu2 \
>    tst-tls23-mod \
>    tst-tlsalign-lib \
> +  tst-tlsalign-tunable-a \

We usually call those auditmods, so tst-tlsalign-tunable-auditmod.

>    tst-tlsgap-mod0 \
>    tst-tlsgap-mod1 \
>    tst-tlsgap-mod2 \
> @@ -1824,6 +1826,12 @@ $(objpfx)tst-nodelete-opened.out: $(objpfx)tst-nodelete-opened-lib.so
>  $(objpfx)tst-tlsalign-extern: $(objpfx)tst-tlsalign-vars.o
>  $(objpfx)tst-tlsalign-extern-static: $(objpfx)tst-tlsalign-vars.o
>  
> +tst-tlsalign-tunable-TUNABLES = glibc.rtld.optional_static_tls_alignment=0x1000
> +tst-tlsalign-tunable-TUNABLES += glibc.rtld.optional_static_tls=0x3000
> +tst-tlsalign-tunable-ENV = LD_AUDIT=$(objpfx)tst-tlsalign-tunable-a.so
> +#tst-tlsalign-tunable-ENV = LD_DEBUG=all

Leftover comment?

> +$(objpfx)tst-tlsalign-tunable.out: $(objpfx)tst-tlsalign-tunable-a.so
> +
>  # The resolver translation unit must always be compiled with
>  # -fstack-protector-all so the canary code is emitted regardless of the
>  # default.
> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> index 1380bd7083..e4c93bace0 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -126,6 +126,8 @@ tls_static_surplus (int nns, int opt_tls)
>     backwards compatibility.  */
>  #define LEGACY_TLS (1664 - tls_static_surplus (DEFAULT_NNS, OPTIONAL_TLS))
>  
> +static size_t optional_static_tls_alignment = 0;

Maybe add a comment where this value comes from?

>  /* 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.  */
> @@ -136,6 +138,7 @@ _dl_tls_static_surplus_init (size_t naudit)
>  
>    nns = TUNABLE_GET (nns, size_t, NULL);
>    opt_tls = TUNABLE_GET (optional_static_tls, size_t, NULL);
> +  optional_static_tls_alignment = TUNABLE_GET (optional_static_tls_alignment, size_t, NULL);

I think this should ignore the value if it's not a power of two.

> diff --git a/elf/tst-tlsalign-tunable-a.c b/elf/tst-tlsalign-tunable-a.c
> new file mode 100644
> index 0000000000..d1a2063e39
> --- /dev/null
> +++ b/elf/tst-tlsalign-tunable-a.c
> @@ -0,0 +1,32 @@
> +#include <unistd.h>
> +
> +__thread int x;
> +__thread int b[4] __attribute__((tls_model("initial-exec"))) __attribute__((aligned(0x1000)));

This line is too long, and there are missing spaces before '('.  There
are more such missing spaces below.

Please also add the usual file header.

> +/* These exist just to make the above variables "used".  */
> +int *
> +func_x(void)
> +{
> +  return &x;
> +}

I guess this is needed because static TLS allocation can in theory be
reference-based.

> diff --git a/elf/tst-tlsalign-tunable.c b/elf/tst-tlsalign-tunable.c
> new file mode 100644
> index 0000000000..3ef2ae2e14
> --- /dev/null
> +++ b/elf/tst-tlsalign-tunable.c

> +static int
> +do_test (void)
> +{
> +  printf("tst-tlsalign-tunable!\n");
> +  /* If the test passes, the auditor itself will call exit with a
> +     value of 0.  If we get here, we've failed.  "Use" tdata1 here
> +     too.  */
> +  return tdata1 ? EXIT_FAILURE : 43;
> +}

I think you need to make tdata1 volatile, otherwise this is still
optimized out (because tdata1 is static and does not have its address
taken).

> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index 713f669c4c..772e55167a 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -422,6 +422,14 @@ changed once allocated at process startup.  The default allocation of
>  optional static TLS is 512 bytes and is allocated in every thread.
>  @end deftp
>  
> +@deftp Tunable glibc.rtld.optional_static_tls_alignment
> +Sets a minimum alignment for the static TLS.  The loader normally uses
> +the alignment requirements of shared objects, but this might be needed
> +for audit modules that have a higher-than-default alignment
> +requirement, as an alternative to requesting excessive
> +optional_static_tls (above).
> +@end deftp

It's not an alternative, both can be needed.  It's also not just about
the auditor, but anything loaded in audit mode counts (whether by the
auditor or anything else).

Thanks,
Florian
  

Florian Weimer <fweimer@redhat.com> writes:
>> +  tst-tlsalign-tunable-a \
>
> We usually call those auditmods, so tst-tlsalign-tunable-auditmod.

Changed.

>> +tst-tlsalign-tunable-ENV = LD_AUDIT=$(objpfx)tst-tlsalign-tunable-a.so
>> +#tst-tlsalign-tunable-ENV = LD_DEBUG=all
>
> Leftover comment?

Yup.

>> +static size_t optional_static_tls_alignment = 0;
>
> Maybe add a comment where this value comes from?

Added.

>> +  optional_static_tls_alignment = TUNABLE_GET (optional_static_tls_alignment, size_t, NULL);
>
> I think this should ignore the value if it's not a power of two.

Done.  No warning messages?

>> +__thread int b[4] __attribute__((tls_model("initial-exec"))) __attribute__((aligned(0x1000)));
>
> This line is too long, and there are missing spaces before '('.  There
> are more such missing spaces below.

Fixed.

> Please also add the usual file header.

Added.

>> +/* These exist just to make the above variables "used".  */
>> +int *
>> +func_x(void)
>> +{
>> +  return &x;
>> +}
>
> I guess this is needed because static TLS allocation can in theory be
> reference-based.

I wanted a way to make sure the compiler didn't omit the tls variable,
but I didn't need it to *use* the tls variable.

>> +  return tdata1 ? EXIT_FAILURE : 43;
>> +}
>
> I think you need to make tdata1 volatile, otherwise this is still
> optimized out (because tdata1 is static and does not have its address
> taken).

Done.

>> +@end deftp
>
> It's not an alternative, both can be needed.  It's also not just about
> the auditor, but anything loaded in audit mode counts (whether by the
> auditor or anything else).

I made the audit case a "for example" and took out the last bit.  It's
hard to describe with simple language how it can be used to optimize
memory needs.