Message-ID: <284293396.70630.1588005648556.JavaMail.zimbra@efficios.com>
Date:   Mon, 27 Apr 2020 12:40:48 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Florian Weimer <fw@...eb.enyo.de>,
        Michael Kerrisk <mtk.manpages@...il.com>
Cc:     libc-alpha <libc-alpha@...rceware.org>, carlos <carlos@...hat.com>,
        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>,
        Szabolcs Nagy <szabolcs.nagy@....com>
Subject: Re: [PATCH glibc 5/9] glibc: Perform rseq(2) registration at C
 startup and thread creation (v17)

----- On Apr 27, 2020, at 5:11 AM, Florian Weimer fw@...eb.enyo.de wrote:

> * Mathieu Desnoyers via Libc-alpha:
> 
>> +* Support for automatically registering threads with the Linux rseq(2)
>> +  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.
>> +  See 'man 2 rseq' for the details of the ABI shared between libc and the
>> +  kernel.
> 
> This should refer documentation in the glibc manual.
> 
> (It is currently a glibc project requirement to add documentation for
> new Linux interfaces, something that I do not necessarily agree with.)

A related issue here is that editing of the rseq(2) man pages has been stalled
since March 2019. I have been waiting for reply from Michael Kerrisk, and I
suspect he might have be side-tracked by other projects. I just bumped that
thread.

Ref.: https://lore.kernel.org/r/211707091.921.1551722548347.JavaMail.zimbra@efficios.com

So as of today, "man 2 rseq" does not exist in the kernel man pages, so I
suggest we remove that sentence. Would the following change be OK with you ?

-  See 'man 2 rseq' for the details of the ABI shared between libc and the
-  kernel.
+  The GNU C Library manual has details on integration of Restartable
+  Sequences.

> 
>>  
>>  Deprecated and removed features, and other changes affecting compatibility:
>>  
>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>> index 1ac66d895d..30466afea0 100644
>> --- a/elf/libc_early_init.c
>> +++ b/elf/libc_early_init.c
>> @@ -18,10 +18,13 @@
>>  
>>  #include <ctype.h>
>>  #include <libc-early-init.h>
>> +#include <rseq-internal.h>
>>  
>>  void
>>  __libc_early_init (void)
>>  {
>>    /* Initialize ctype data.  */
>>    __ctype_init ();
>> +  /* Register rseq ABI to the kernel.   */
>> +  (void) rseq_register_current_thread ();
>>  }
> 
> The cast to void should be removed (see below the comment about the
> return type.

OK

> 
>> diff --git a/manual/threads.texi b/manual/threads.texi
>> index 0858ef8f92..59f634e432 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
>>  
>>  
>> @@ -881,3 +883,27 @@ 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 rseq
>> +
>> +This section describes @theglibc{} Restartable Sequences integration.
>> +
>> +@...typevar {struct rseq} __rseq_abi
>> +@...ndards{GNU, 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 Linux kernel @file{linux/rseq.h} UAPI.
>> +Registration of each thread's @code{__rseq_abi} is performed by
>> +@...glibc{} at libc initialization and pthread creation.
>> +@end deftypevar
>> +
>> +@...typevr Macro int RSEQ_SIG
>> +@...ndards{GNU, sys/rseq.h}
>> +Each supported architecture provide a @code{RSEQ_SIG} macro in
>> +@...e{sys/rseq.h} which contains a signature.  That signature is expected to be
>> +present in the code before each Restartable Sequences abort handler.  Failure
>> +to provide the expected signature may terminate the process with a Segmentation
>> +fault.
>> +@end deftypevr
> 
> This should say Linux, not GNU as the standards source, given that the
> interface is not added to the GNU ABI.

Good point, done.

> 
> Is this sufficient documentation of this feature?  What do others
> think about this?

I'll leave this question to others.

> 
>> diff --git a/misc/rseq-internal.h b/misc/rseq-internal.h
>> new file mode 100644
>> index 0000000000..d564cf1bc3
>> --- /dev/null
>> +++ b/misc/rseq-internal.h
>> @@ -0,0 +1,33 @@
> 
>> +static inline int
>> +rseq_register_current_thread (void)
>> +{
>> +  return -1;
>> +}
> 
> Nothing checks the return value of this function as far as I can see,
> so it can return void.

OK

> 
>> +static inline int
>> +rseq_unregister_current_thread (void)
>> +{
>> +  return -1;
>> +}
> 
> This function is unused.  This also applies to the full version.  I
> believe we switched to implicit deregistration on thread exit, so I
> think you can just remove it.

OK

> 
>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
>> index afd379e89a..1ff248042e 100644
>> --- a/nptl/pthread_create.c
>> +++ b/nptl/pthread_create.c
>> @@ -33,6 +33,7 @@
>>  #include <default-sched.h>
>>  #include <futex-internal.h>
>>  #include <tls-setup.h>
>> +#include <rseq-internal.h>
>>  #include "libioP.h"
>>  
>>  #include <shlib-compat.h>
>> @@ -384,6 +385,9 @@ START_THREAD_DEFN
>>    /* Initialize pointers to locale data.  */
>>    __ctype_init ();
>>  
>> +  /* Register rseq TLS to the kernel. */
>> +  (void) rseq_register_current_thread ();
>> +
> 
> The cast can go.

OK

> 
>>  #ifndef __ASSUME_SET_ROBUST_LIST
>>    if (__set_robust_list_avail >= 0)
>>  #endif
>> @@ -578,6 +582,14 @@ START_THREAD_DEFN
>>       process is really dead since 'clone' got passed the CLONE_CHILD_CLEARTID
>>       flag.  The 'tid' field in the TCB will be set to zero.
>>  
>> +     rseq TLS is still registered at this point. Rely on implicit
>> unregistration
>> +     performed by the kernel on thread teardown. This is not a problem because
>> the
>> +     rseq TLS lives on the stack, and the stack outlives the thread. If TCB
>> +     allocation is ever changed, additional steps may be required, such as
>> +     performing explicit rseq unregistration before reclaiming the rseq TLS
>> area
>> +     memory. It is NOT sufficient to block signals because the kernel may write
>> +     to the rseq area even without signals.
>> +
>>       The exit code is zero since in case all threads exit by calling
>>       'pthread_exit' the exit status must be 0 (zero).  */
>>    __exit_thread ();
> 
> Some of these lines are too long.  Also two spaces after . at the end
> of sentences.

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..5f7f02f1ec
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/rseq-internal.h
> 
>> +static inline int
>> +rseq_register_current_thread (void)
>> +{
>> +  int rc, ret = 0;
>> +
>> +  if (__rseq_abi.cpu_id == RSEQ_CPU_ID_REGISTRATION_FAILED)
>> +    return -1;
>> +  rc = INTERNAL_SYSCALL_CALL (rseq, &__rseq_abi, sizeof (struct rseq),
>> +                              0, RSEQ_SIG);
>> +  if (!rc)
>> +    goto end;
>> +  if (INTERNAL_SYSCALL_ERRNO (rc) != EBUSY)
>> +    __rseq_abi.cpu_id = RSEQ_CPU_ID_REGISTRATION_FAILED;
>> +  ret = -1;
>> +end:
>> +  return ret;
>> +}
> 
> This does not seem to use INTERNAL_SYSCALL_CALL correctly.  I think
> you need to use INTERNAL_SYSCALL_ERROR_P on the result to check for an
> error, and only then use INTERNAL_SYSCALL_ERRNO to extract the error
> code.

OK

> 
>> diff --git a/sysdeps/unix/sysv/linux/rseq-sym.c
>> b/sysdeps/unix/sysv/linux/rseq-sym.c
>> new file mode 100644
>> index 0000000000..0e33fab278
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/rseq-sym.c
> 
>> +#include <sys/syscall.h>
>> +#include <stdint.h>
>> +#include <kernel-features.h>
>> +#include <sys/rseq.h>
>> +
>> +__thread struct rseq __rseq_abi = {
>> +  .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
>> +};
> 
> { should go onto its own line.

OK

> I'd also add attribute_tls_model_ie,
> also it's implied by the declaration in the header.

This contradicts feedback I received from Szabolcs Nagy in September 2019:

https://public-inbox.org/libc-alpha/c58d4d6e-f22a-f5d9-e23a-5bd72cec1a86@arm.com/

"note that libpthread.so is built with -ftls-model=initial-exec

(and if it wasn't then you'd want to put the attribute on the
declaration in the internal header file, not on the definition,
so the actual tls accesses generate the right code)"

In the context of his feedback, __rseq_abi was defined within nptl/pthread_create.c.
It is now defined in sysdeps/unix/sysv/linux/rseq-sym.c, which is built into the
csu which AFAIU ends up in libc.so. His comment still applies though, because
libc.so is also built with -ftls-model=initial-exec.

So should I apply the "initial-exec" TLS model only to the __rseq_abi
declaration, or is it preferred to apply it to both the declaration
and the definition ?

> 
>> diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h
>> b/sysdeps/unix/sysv/linux/sys/rseq.h
>> new file mode 100644
>> index 0000000000..503dce4cac
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/sys/rseq.h
>> @@ -0,0 +1,186 @@
> 
> I think there is some value in making this header compatible with
> inclusion from the assembler (including constants for the relevant
> struct offsets), but that can be a later change.

Agreed. By "later", do you mean before merging the patch, between
merge of the patch and next glibc release, or for a subsequent glibc
release ?

> 
>> +#ifndef _SYS_RSEQ_H
>> +#define _SYS_RSEQ_H	1
>> +
>> +/* Architecture-specific rseq signature.  */
>> +#include <bits/rseq.h>
> 
> Maybe add a newline between the above and the following, to make clear
> the comment only applies to the first #include.

OK

> 
>> +#include <stdint.h>
>> +#include <sys/cdefs.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
>> +
>> +#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.  */
>> +
>> +#include <asm/byteorder.h>
>> +
>> +enum rseq_cpu_id_state
>> +  {
>> +    RSEQ_CPU_ID_UNINITIALIZED = -1,
>> +    RSEQ_CPU_ID_REGISTRATION_FAILED = -2,
>> +  };
>> +
>> +enum rseq_flags
>> +  {
>> +    RSEQ_FLAG_UNREGISTER = (1 << 0),
>> +  };
>> +
>> +enum rseq_cs_flags_bit
>> +  {
>> +    RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT_BIT = 0,
>> +    RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT = 1,
>> +    RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT = 2,
>> +  };
>> +
>> +enum rseq_cs_flags
>> +  {
>> +    RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT =
>> +      (1U << RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT_BIT),
>> +    RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL =
>> +      (1U << RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT),
>> +    RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE =
>> +      (1U << RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT),
>> +  };
>> +
>> +/* struct rseq_cs is aligned on 4 * 8 bytes to ensure it is always
>> +   contained within a single cache-line. It is usually declared as
>> +   link-time constant data.  */
>> +struct rseq_cs
>> +  {
>> +    /* Version of this structure.  */
>> +    uint32_t version;
>> +    /* enum rseq_cs_flags.  */
>> +    uint32_t flags;
>> +    uint64_t start_ip;
>> +    /* Offset from start_ip.  */
>> +    uint64_t post_commit_offset;
>> +    uint64_t abort_ip;
>> +} __attribute__((aligned(4 * sizeof(uint64_t))));
> 
> The comment is wrong.  32-byte alignment does not put struct rseq_cs
> on its own cache line on many (most?) CPUs.  Not using the constant 32
> looks like unnecessary obfuscation to me.

There is a difference between "being contained within a single cache-line"
and "being the only structure in a cache-line". The objective here is the
former.

For instance, if we do not enforce any minimum alignment, the compiler could
decide to align that structure on __alignof__(uint64_t) which happens to be
4 bytes on some architectures. This can cause this frequently accessed structure
to be sitting across 2 cache-lines, thus requiring the CPU to load two cache
lines rather than one very frequently.

I think what you have in mind is "being the only structure in a cache-line",
which is useful to eliminate false-sharing. However, considering that this is
a TLS variable, we don't care about false-sharing, because it is never meant
to be updated concurrently by many threads or CPUs.

So I think my comment is correct.

I agree that the constant 32 may be clearer here. I will change to align(32).

> I still think we should avoid the alignment.  The _ip fields should
> perhaps be _pc (IP is more or less specific to x86).

I am concerned that removing an alignment attribute which is exposed
in a public Linux UAPI header can be an ABI breakage.

I am also concerned about changing field names for fields already
exposed in a public Linux UAPI header, especially if the change is
only for cosmetic reasons.

> 
> { and } are not aligned.  Please do not forget to add spaces before
> opening parentheses, and two spaces after the . and the end of
> sentences.  The opening { should always be on its own line.  (This
> also applies to the definition of struct rseq below.)

OK. Old coding style habits die hard ;)

> 
>> +
>> +/* struct rseq is aligned on 4 * 8 bytes to ensure it is always
>> +   contained within a single cache-line.
>> +
>> +   A single struct rseq per thread is allowed.  */
>> +struct rseq
>> +  {
>> +    /* Restartable sequences cpu_id_start field. Updated by the
>> +       kernel. Read by user-space with single-copy atomicity
>> +       semantics. This field should only be read by the thread which
>> +       registered this data structure. Aligned on 32-bit. Always
> 
> What does “Aligned on 32-bit” mean in this context?  Do you mean to
> reference 32-*byte* alignment here?

No. I really mean 32-bit (4-byte). Being aligned on 32-byte guarantees that
this field is aligned at least on 4-byte. This is required by single-copy
atomicity semantics.

Should I update this comment to state "Aligned on 4-byte" instead ?

> 
>> +    /* Restartable sequences rseq_cs field.
>> +
>> +       Contains NULL when no critical section is active for the current
>> +       thread, or holds a pointer to the currently active struct rseq_cs.
>> +
>> +       Updated by user-space, which sets the address of the currently
>> +       active rseq_cs at the beginning of assembly instruction sequence
>> +       block, and set to NULL by the kernel when it restarts an assembly
>> +       instruction sequence block, as well as when the kernel detects that
>> +       it is preempting or delivering a signal outside of the range
>> +       targeted by the rseq_cs. Also needs to be set to NULL by user-space
>> +       before reclaiming memory that contains the targeted struct rseq_cs.
>> +
>> +       Read and set by the kernel. Set by user-space with single-copy
>> +       atomicity semantics. This field should only be updated by the
>> +       thread which registered this data structure. Aligned on 64-bit.  */
>> +    union {
>> +      uint64_t ptr64;
>> +#ifdef __LP64__
>> +      uint64_t ptr;
>> +#else
>> +      struct {
>> +#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) ||
>> defined(__BIG_ENDIAN)
>> +        uint32_t padding; /* Initialized to zero.  */
>> +        uint32_t ptr32;
>> +#else /* LITTLE */
>> +        uint32_t ptr32;
>> +        uint32_t padding; /* Initialized to zero.  */
>> +#endif /* ENDIAN */
>> +      } ptr;
>> +#endif
>> +    } rseq_cs;
> 
> Are these conditionals correct for x32?

Let's see. With x86 gcc:

-m64: (__x86_64__ && __LP64__)
-m32: (__i386__)
-mx32: (__x86_64__ && __ILP32__)

So with "#ifdef __LP64__" we specifically target 64-bit pointers. The rest
falls into the "else" case, which expects 32-bit pointers. Considering that
x32 has 32-bit pointers, I don't see any issue here.

> Shouldn't there be a member of type const struct rseq_cs * somewhere?

Having pointers within structures in kernel UAPI headers is frowned upon. Indeed
here having it in the union could possibly make some use-cases easier in
user-space, so I'm open to it. It basically depends on how much we want the
Linux UAPI header and the glibc header to stay in sync, and if other kernel
maintainers are open to this addition.

We don't mind that user-space uses that pointer, but we never want the kernel
to touch that pointer rather than the 32/64-bit-aware fields. One possibility
would be to do:

    union
      {
        uint64_t ptr64;
#ifdef __LP64__
        uint64_t ptr;
#else
        struct
          {
#if (defined (__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined (__BIG_ENDIAN)
            uint32_t padding; /* Initialized to zero.  */
            uint32_t ptr32;
#else /* LITTLE */
            uint32_t ptr32;
            uint32_t padding; /* Initialized to zero.  */
#endif /* ENDIAN */
          } ptr;
#endif

#ifndef __KERNEL__
     const struct rseq_cs *uptr;
#endif
      } rseq_cs;

in the union, so only user-space can see that field. Thoughts ?

> 
>> diff --git a/sysdeps/unix/sysv/linux/x86/bits/rseq.h
>> b/sysdeps/unix/sysv/linux/x86/bits/rseq.h
>> new file mode 100644
>> index 0000000000..75f52d9788
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/x86/bits/rseq.h
>> @@ -0,0 +1,30 @@
>> +/* Restartable Sequences Linux x86 architecture header.
>> +   Copyright (C) 2019-2020 Free Software Foundation, Inc.
> 
> Please make sure that none of the new files reference the 2019 year.
> It should be 2020, per GNU policy.

OK

> 
> The patch needs some rebasing on top of current master.

Done.

Thanks for the review!

Mathieu


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

Powered by blists - more mailing lists