Date:   Mon, 25 May 2020 10:51:45 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Florian Weimer <fweimer@...hat.com>
Cc:     libc-alpha <libc-alpha@...rceware.org>,
        Rich Felker <dalias@...c.org>,
        linux-api <linux-api@...r.kernel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Will Deacon <will.deacon@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ben Maurer <bmaurer@...com>, Dave Watson <davejwatson@...com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Paul <paulmck@...ux.vnet.ibm.com>, Paul Turner <pjt@...gle.com>,
        Joseph Myers <joseph@...esourcery.com>
Subject: Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup
 and thread creation (v19)

----- On May 20, 2020, at 7:40 AM, Florian Weimer fweimer@...hat.com wrote:

> * 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)”.

OK

[...]
> 
>> 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).

OK

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

OK

> 
>> @@ -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
>> +
>> +@...e Restartable Sequences
>> +@...tion Restartable Sequences
>> +@...dex Restartable Sequences
>> +
>> +This section describes Restartable Sequences integration for
>> +@...glibc{}.
> 
> “This functionality is only available on Linux.”  (The @standards parts
> are not visible to readers.)

OK

> 
>> +
>> +@...typevar {struct rseq} __rseq_abi
>> +@...ndards{Linux, sys/rseq.h}
>> +@...glibc{} 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.

OK

> 
>> 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!).

Sorry! My bad!

> 
>> 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.

At least I was consistently wrong. ;-)

> 
>> 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.)

I would prefer we keep this for a later patch.

That being said, I notice that gcc prior to 4.9 has trouble compiling
__has_include. Therefore, I would tend to recommend the following as a
backward-compatible way of using the rseq header from external projects:

- At configure time, check for availability of sys/rseq.h to ensure the
  glibc version is recent enough. In the autotools world, this is usually
  done with a small test program which will fail to build if the header is
  not there.
- At preprocessing time, include <sys/rseq.h> and check whether RSEQ_SIG
  is defined.

If a project only aims at building with gcc 4.9 or newer, then it can use
__has_include.

> 
>> 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.

OK

> 
>> 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.

OK

> 
>> +#else
>> +static inline void
>> +rseq_register_current_thread (void)
>> +{
>> +}
>> +#endif
> 
> Maybe add /* RSEQ_SIG */ comments to #else/#endif here as well.

OK

> 
>> 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.

OK

> 
> 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.

Would the plan be to only do the #include <bits/rseq.h> bits in #ifdef
__ASSEMBLER__ and skip all the rest ?

> 
>> +#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.

Ah, I get to your uptr concern now! Good :)

The larger question here is: considering that we re-implement the entire
uapi header within glibc (which includes the uptr addition), do we still
care about using the header provided by the Linux kernel ?

Having different definitions depending on whether a kernel header is
installed or not when including a glibc header seems rather unexpected.

*If* we want to use the uapi header, I think something is semantically
missing. Here is the scheme I envision. We could rely on the kernel header
version.h to figure out which of glibc or kernel uapi header is more
recent. Any new concept we try to integrate into glibc (e.g. uptr)
should go into the upstream Linux uapi header first.

For the coming glibc e.g. 2.32, we use the kernel uapi header if
kernel version is >= 4.18.0. Within glibc, the fallback implements
exactly the API exposed by the kernel rseq.h header.

As we eventually introduce the uptr change into the Linux kernel, and
say it gets merged for Linux 5.9.0, we mirror this change into glibc
(e.g. release 2.33), and bump the Linux kernel version cutoff to 5.9.0.
So starting from that version, we use the Linux kernel header only if
version >= 5.9.0, else we fallback on glibc's own implementation.

This is clearly something we need to get right.

> 
>> +/* 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.

Would the following be ok ?

#ifdef __cplusplus
#define rseq_static_assert      static_assert
#else
#define rseq_static_assert      _Static_assert
#endif

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

> 
>> +/* 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__.

OK

> 
> We're getting really close now. 8-)

Great!

Thanks for the feedback!

Mathieu

> 
> Thanks,
> Florian

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists