Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19)

[Florian Weimer <fweimer@redhat.com>]

* Mathieu Desnoyers via Libc-alpha:

> diff --git a/NEWS b/NEWS
> index 141078c319..c4e0370fc4 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,6 +23,16 @@ Major new features:
>    toolchains.  It is recommended to use GCC 8 or newer when testing
>    this option.
>  
> +* Support for automatically registering threads with the Linux rseq
> +  system call has been added.  This system call is implemented starting
> +  from Linux 4.18.  The Restartable Sequences ABI accelerates user-space
> +  operations on per-cpu data.  It allows user-space to perform updates
> +  on per-cpu data without requiring heavy-weight atomic operations.
> +  Automatically registering threads allows all libraries, including libc,
> +  to make immediate use of the rseq(2) support by using the documented ABI.
> +  The GNU C Library manual has details on integration of Restartable
> +  Sequences.

“rseq” instead “rseq(2)”.

> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
> index e6c64fb526..f0fcf6448e 100644
> --- a/elf/libc_early_init.c
> +++ b/elf/libc_early_init.c
> @@ -18,10 +18,14 @@
>  
>  #include <ctype.h>
>  #include <libc-early-init.h>
> +#include <rseq-internal.h>
>  
>  void
>  __libc_early_init (_Bool initial)
>  {
>    /* Initialize ctype data.  */
>    __ctype_init ();
> +  /* Register rseq ABI to the kernel for the main program's libc.   */
> +  if (initial)
> +    rseq_register_current_thread ();
>  }

Okay.

> diff --git a/manual/threads.texi b/manual/threads.texi
> index 0858ef8f92..a565095c43 100644
> --- a/manual/threads.texi
> +++ b/manual/threads.texi
> @@ -9,8 +9,10 @@ This chapter describes functions used for managing threads.
>  POSIX threads.
>  
>  @menu
> -* ISO C Threads::	Threads based on the ISO C specification.
> -* POSIX Threads::	Threads based on the POSIX specification.
> +* ISO C Threads::		Threads based on the ISO C specification.
> +* POSIX Threads::		Threads based on the POSIX specification.
> +* Restartable Sequences::	Linux-specific Restartable Sequences
> +				integration.
>  @end menu

This should go into the extensions menu (@node Non-POSIX Extensions).

General comment: Please wrap the lines around 72 or so characters.
Thanks.

> @@ -881,3 +883,63 @@ Behaves like @code{pthread_timedjoin_np} except that the absolute time in
>  @c pthread_spin_unlock
>  @c pthread_testcancel
>  @c pthread_yield
> +
> +@node Restartable Sequences
> +@section Restartable Sequences
> +@cindex Restartable Sequences
> +
> +This section describes Restartable Sequences integration for
> +@theglibc{}.

“This functionality is only available on Linux.”  (The @standards parts
are not visible to readers.)

> +
> +@deftypevar {struct rseq} __rseq_abi
> +@standards{Linux, sys/rseq.h}
> +@Theglibc{} implements a @code{__rseq_abi} TLS symbol to interact with the
> +Restartable Sequences system call (Linux-specific).  The layout of this
> +structure is defined by the @file{sys/rseq.h} header.  Registration of each
> +thread's @code{__rseq_abi} is performed by @theglibc{} at library
> +initialization and thread creation.

Can drop “(Linux-specific)” here.

> diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h
> new file mode 100644
> index 0000000000..37d83fcb4a
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h

> +#ifdef __AARCH64EB__
> +#define RSEQ_SIG_DATA	0x00bc28d4	/* BRK #0x45E0.  */
> +#else
> +#define RSEQ_SIG_DATA	RSEQ_SIG_CODE
> +#endif

Missing indentation on the #defines (sorry!).

> diff --git a/sysdeps/unix/sysv/linux/arm/bits/rseq.h b/sysdeps/unix/sysv/linux/arm/bits/rseq.h
> new file mode 100644
> index 0000000000..c132f0327c
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/arm/bits/rseq.h

> +#ifdef __ARMEB__
> +#define RSEQ_SIG    0xf3def5e7      /* udf    #24035    ; 0x5de3 (ARMv6+) */
> +#else
> +#define RSEQ_SIG    0xe7f5def3      /* udf    #24035    ; 0x5de3 */
> +#endif

Likewise, missing indentation.

> diff --git a/sysdeps/unix/sysv/linux/bits/rseq.h b/sysdeps/unix/sysv/linux/bits/rseq.h
> new file mode 100644
> index 0000000000..014c08fe0f
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/bits/rseq.h
> @@ -0,0 +1,29 @@

> +/* RSEQ_SIG is a signature required before each abort handler code.
> +
> +   It is a 32-bit value that maps to actual architecture code compiled
> +   into applications and libraries.  It needs to be defined for each
> +   architecture.  When choosing this value, it needs to be taken into
> +   account that generating invalid instructions may have ill effects on
> +   tools like objdump, and may also have impact on the CPU speculative
> +   execution efficiency in some cases.  */

I wonder if we should say something somewhere that the correct way to
check for compile-time rseq support in glibc is something like this?

#if __has_include (<sys/rseq.h>)
# include <sys/rseq.h>
#endif
#ifdef RSEQ_SIG
…
#endif

Or perhaps we should suppress installation of the headers if we only
have support for the stub.

(I think this fine tuning can be deferred to later patch.)

> diff --git a/sysdeps/unix/sysv/linux/mips/bits/rseq.h b/sysdeps/unix/sysv/linux/mips/bits/rseq.h
> new file mode 100644
> index 0000000000..cbad4290cc
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/mips/bits/rseq.h
> @@ -0,0 +1,62 @@

> +#if defined(__nanomips__)
> +# ifdef __MIPSEL__
> +#  define RSEQ_SIG	0x03500010
> +# else
> +#  define RSEQ_SIG	0x00100350
> +# endif
> +#elif defined(__mips_micromips)
> +# ifdef __MIPSEL__
> +#  define RSEQ_SIG	0xd4070000
> +# else
> +#  define RSEQ_SIG	0x0000d407
> +# endif
> +#elif defined(__mips__)
> +# define RSEQ_SIG	0x0350000d
> +#else
> +/* Unknown MIPS architecture.  */
> +#endif

Please use “defined (”, with a space.

> diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h b/sysdeps/unix/sysv/linux/rseq-internal.h
> new file mode 100644
> index 0000000000..6583691285
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/rseq-internal.h

> +#ifdef RSEQ_SIG
> +static inline void
> +rseq_register_current_thread (void)

> +      if (msg)
> +        __libc_fatal (msg);
> +    }

“if (msg != NULL)”, please.

> +#else
> +static inline void
> +rseq_register_current_thread (void)
> +{
> +}
> +#endif

Maybe add /* RSEQ_SIG */ comments to #else/#endif here as well.

> diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h b/sysdeps/unix/sysv/linux/sys/rseq.h
> new file mode 100644
> index 0000000000..ea51194bf8
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/sys/rseq.h

> +#ifdef __has_include
> +# if __has_include ("linux/rseq.h")
> +#   define __GLIBC_HAVE_KERNEL_RSEQ
> +# endif
> +#else
> +# include <linux/version.h>
> +# if LINUX_VERSION_CODE >= KERNEL_VERSION (4, 18, 0)
> +#   define __GLIBC_HAVE_KERNEL_RSEQ
> +# endif
> +#endif

Too much indentation on the define line, I think.

Still missing: #ifdef __ASSEMBLER__.  .S files should be able to include
<sys/rseq.h> to get the definition of RSEQ_SIG.  But I think this can be
deferred to a follow-up.

> +#ifdef __GLIBC_HAVE_KERNEL_RSEQ
> +/* We use the structures declarations from the kernel headers.  */
> +# include <linux/rseq.h>
> +#else
> +/* We use a copy of the include/uapi/linux/rseq.h kernel header.  */

This comment is not true, the kernel headers do not have uptr support.

If we revert the uptr change, we also need to update the manual, I
think.

> +/* Ensure the compiler supports __attribute__ ((aligned)).  */
> +_Static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment");
> +_Static_assert (__alignof__ (struct rseq) >= 32, "alignment");

This needs #ifndef __cplusplus or something like that.  I'm surprised
that this passes the installed header tests.

> +/* Allocations of struct rseq and struct rseq_cs on the heap need to
> +   be aligned on 32 bytes.  Therefore, use of malloc is discouraged
> +   because it does not guarantee alignment.  posix_memalign should be
> +   used instead.  */
> +
> +extern __thread struct rseq __rseq_abi
> +  __attribute__ ((tls_model ("initial-exec")));

Should be __tls_model__.

We're getting really close now. 8-)

Thanks,
Florian