[4/4] rseq registration tests (v2)

These tests validate that rseq is registered from various execution
contexts (main thread, constructor, destructor, other threads, other
threads created from constructor and destructor, forked process
(without exec), pthread_atfork handlers, pthread setspecific
destructors, C++ thread and process destructors, signal handlers,
atexit handlers).

tst-rseq.c only links against libc.so, testing registration of rseq in
a non-multithreaded environment.

tst-rseq-nptl.c also links against libpthread.so, testing registration
of rseq in a multithreaded environment.

See the Linux kernel selftests for extensive rseq stress-tests.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Carlos O'Donell <carlos@redhat.com>
CC: Florian Weimer <fweimer@redhat.com>
CC: Joseph Myers <joseph@codesourcery.com>
CC: Szabolcs Nagy <szabolcs.nagy@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ben Maurer <bmaurer@fb.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Dave Watson <davejwatson@fb.com>
CC: Paul Turner <pjt@google.com>
CC: libc-alpha@sourceware.org
---
Changes since v1:
- Rename tst-rseq.c to tst-rseq-nptl.c.
- Introduce tst-rseq.c testing rseq registration in a non-multithreaded
  environment.
---
 sysdeps/unix/sysv/linux/Makefile        |   4 +-
 sysdeps/unix/sysv/linux/tst-rseq-nptl.c | 381 ++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/tst-rseq.c      | 110 +++++++
 3 files changed, 493 insertions(+), 2 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-rseq-nptl.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-rseq.c

On 2/12/19 2:42 PM, Mathieu Desnoyers wrote:
> These tests validate that rseq is registered from various execution
> contexts (main thread, constructor, destructor, other threads, other
> threads created from constructor and destructor, forked process
> (without exec), pthread_atfork handlers, pthread setspecific
> destructors, C++ thread and process destructors, signal handlers,
> atexit handlers).

Thanks, this set of tests looks good, and covers the main uses.

> tst-rseq.c only links against libc.so, testing registration of rseq in
> a non-multithreaded environment.

OK.

> tst-rseq-nptl.c also links against libpthread.so, testing registration
> of rseq in a multithreaded environment.

OK.

> See the Linux kernel selftests for extensive rseq stress-tests.

Yeah, that's where they should be, here we just want to exercise the
basic set of tests to ensure that integration with glibc is working
as expected.

This patch looks good to me, but is blocked on patch 1 acceptance,

Cleanup the following:
- One line short dedscriptions for files.
- Removal of contributed by lines.
- Answer small thread stack question.
and I'd say we're ready with this patch too.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: Carlos O'Donell <carlos@redhat.com>
> CC: Florian Weimer <fweimer@redhat.com>
> CC: Joseph Myers <joseph@codesourcery.com>
> CC: Szabolcs Nagy <szabolcs.nagy@arm.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ben Maurer <bmaurer@fb.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> CC: Boqun Feng <boqun.feng@gmail.com>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Dave Watson <davejwatson@fb.com>
> CC: Paul Turner <pjt@google.com>
> CC: libc-alpha@sourceware.org
> ---
> Changes since v1:
> - Rename tst-rseq.c to tst-rseq-nptl.c.
> - Introduce tst-rseq.c testing rseq registration in a non-multithreaded
>    environment.
> ---
>   sysdeps/unix/sysv/linux/Makefile        |   4 +-
>   sysdeps/unix/sysv/linux/tst-rseq-nptl.c | 381 ++++++++++++++++++++++++
>   sysdeps/unix/sysv/linux/tst-rseq.c      | 110 +++++++
>   3 files changed, 493 insertions(+), 2 deletions(-)
>   create mode 100644 sysdeps/unix/sysv/linux/tst-rseq-nptl.c
>   create mode 100644 sysdeps/unix/sysv/linux/tst-rseq.c
> 
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 5b541469ec..5f69f644a8 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -53,7 +53,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
>   tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
>   	 tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
>   	 test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
> -	 tst-rlimit-infinity tst-ofdlocks
> +	 tst-rlimit-infinity tst-ofdlocks tst-rseq

OK.

>   tests-internal += tst-ofdlocks-compat
>   
>   
> @@ -230,5 +230,5 @@ ifeq ($(subdir),nptl)
>   tests += tst-align-clone tst-getpid1 \
>   	tst-thread-affinity-pthread tst-thread-affinity-pthread2 \
>   	tst-thread-affinity-sched
> -tests-internal += tst-setgetname
> +tests-internal += tst-setgetname tst-rseq-nptl

OK.

>   endif
> diff --git a/sysdeps/unix/sysv/linux/tst-rseq-nptl.c b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c
> new file mode 100644
> index 0000000000..f4be4d2ae1
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c
> @@ -0,0 +1,381 @@

Need a one line short description.

> +/* Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.

As noted in the other reviews, please remove the contributed by lines.

> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* These tests validate that rseq is registered from various execution
> +   contexts (main thread, constructor, destructor, other threads, other
> +   threads created from constructor and destructor, forked process
> +   (without exec), pthread_atfork handlers, pthread setspecific
> +   destructors, C++ thread and process destructors, signal handlers,
> +   atexit handlers).
> +
> +   See the Linux kernel selftests for extensive rseq stress-tests.  */

OK.

> +
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +
> +#ifdef __NR_rseq
> +#define HAS_RSEQ
> +#endif
> +
> +#ifdef HAS_RSEQ
> +#include <sys/rseq.h>
> +#include <pthread.h>
> +#include <syscall.h>
> +#include <stdlib.h>
> +#include <error.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <signal.h>
> +#include <atomic.h>
> +
> +static pthread_key_t rseq_test_key;

OK.

> +
> +static int
> +rseq_thread_registered (void)
> +{
> +  return (int32_t) __rseq_abi.cpu_id >= 0;

OK.

> +}
> +
> +static int
> +do_rseq_main_test (void)
> +{
> +  if (raise (SIGUSR1))
> +    FAIL_EXIT1 ("error raising signal");
> +  if (pthread_setspecific (rseq_test_key, (void *) 1l))
> +    FAIL_EXIT1 ("error in pthread_setspecific");
> +  if (!rseq_thread_registered ())
> +    {
> +      FAIL_RET ("rseq not registered in main thread");

OK.

> +    }
> +  return 0;
> +}
> +
> +static void
> +cancel_routine (void *arg)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      printf ("rseq not registered in cancel routine\n");
> +      support_record_failure ();
> +    }
> +}

OK.

> +
> +static int cancel_thread_ready;
> +
> +static void
> +test_cancel_thread (void)
> +{
> +  pthread_cleanup_push (cancel_routine, NULL);
> +  atomic_store_release (&cancel_thread_ready, 1);
> +  for (;;)
> +    usleep (100);
> +  pthread_cleanup_pop (0);


OK.

> +}
> +
> +static void *
> +thread_function (void * arg)
> +{
> +  int i = (int) (intptr_t) arg;
> +
> +  if (raise (SIGUSR1))
> +    FAIL_EXIT1 ("error raising signal");
> +  if (i == 0)
> +    test_cancel_thread ();
> +  if (pthread_setspecific (rseq_test_key, (void *) 1l))
> +    FAIL_EXIT1 ("error in pthread_setspecific");
> +  return rseq_thread_registered () ? NULL : (void *) 1l;
> +}

OK.

> +
> +static void
> +sighandler (int sig)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      printf ("rseq not registered in signal handler\n");
> +      support_record_failure ();
> +    }
> +}

OK.

> +
> +static void
> +setup_signals (void)
> +{
> +  struct sigaction sa;
> +
> +  sigemptyset (&sa.sa_mask);
> +  sigaddset (&sa.sa_mask, SIGUSR1);
> +  sa.sa_flags = 0;
> +  sa.sa_handler = sighandler;
> +  if (sigaction (SIGUSR1, &sa, NULL) != 0)
> +    {
> +      FAIL_EXIT1 ("sigaction failure: %s", strerror (errno));

OK.

> +    }
> +}
> +
> +#define N 7
> +static const int t[N] = { 1, 2, 6, 5, 4, 3, 50 };

OK.

> +
> +static int
> +do_rseq_threads_test (int nr_threads)
> +{
> +  pthread_t th[nr_threads];
> +  int i;
> +  int result = 0;
> +  pthread_attr_t at;
> +
> +  if (pthread_attr_init (&at) != 0)
> +    {
> +      FAIL_EXIT1 ("attr_init failed");
> +    }
> +
> +  if (pthread_attr_setstacksize (&at, 1 * 1024 * 1024) != 0)

Why set it that small? Why set it at all?

> +    {
> +      FAIL_EXIT1 ("attr_setstacksize failed");
> +    }
> +
> +  cancel_thread_ready = 0;
> +  for (i = 0; i < nr_threads; ++i)
> +    if (pthread_create (&th[i], NULL, thread_function,
> +                        (void *) (intptr_t) i) != 0)
> +      {
> +        FAIL_EXIT1 ("creation of thread %d failed", i);
> +      }
> +
> +  if (pthread_attr_destroy (&at) != 0)
> +    {
> +      FAIL_EXIT1 ("attr_destroy failed");
> +    }
> +
> +  while (!atomic_load_acquire (&cancel_thread_ready))
> +    usleep (100);
> +
> +  if (pthread_cancel (th[0]))
> +    FAIL_EXIT1 ("error in pthread_cancel");
> +
> +  for (i = 0; i < nr_threads; ++i)
> +    {
> +      void *v;
> +      if (pthread_join (th[i], &v) != 0)
> +        {
> +          printf ("join of thread %d failed\n", i);
> +          result = 1;
> +        }
> +      else if (i != 0 && v != NULL)
> +        {
> +          printf ("join %d successful, but child failed\n", i);
> +          result = 1;
> +        }
> +      else if (i == 0 && v == NULL)
> +        {
> +          printf ("join %d successful, child did not fail as expected\n", i);
> +          result = 1;
> +        }
> +    }
> +  return result;
> +}
> +
> +static int
> +sys_rseq (volatile struct rseq *rseq_abi, uint32_t rseq_len,
> +          int flags, uint32_t sig)
> +{
> +  return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig);

OK. Decided in other mail that rseq should not be public.

> +}
> +
> +static int
> +rseq_available (void)
> +{
> +  int rc;
> +
> +  rc = sys_rseq (NULL, 0, 0, 0);
> +  if (rc != -1)
> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
> +  switch (errno)
> +    {
> +    case ENOSYS:
> +      return 0;
> +    case EINVAL:
> +      return 1;
> +    default:
> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
> +    }

OK

> +}
> +
> +static int
> +do_rseq_fork_test (void)
> +{
> +  int status;
> +  pid_t pid, retpid;
> +
> +  pid = fork ();
> +  switch (pid)
> +    {
> +      case 0:
> +        exit (do_rseq_main_test ());
> +      case -1:
> +        FAIL_EXIT1 ("Unexpected fork error %s", strerror (errno));
> +    }
> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
> +  if (retpid != pid)
> +    {
> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
> +                  (long int) retpid, (long int) pid);
> +    }
> +  if (WEXITSTATUS (status))
> +    {
> +      printf ("rseq not registered in child\n");
> +      return 1;
> +    }
> +  return 0;

OK.

> +}
> +
> +static int
> +do_rseq_test (void)
> +{
> +  int i, result = 0;
> +
> +  if (!rseq_available ())
> +    {
> +      FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test");

OK.

> +    }
> +  setup_signals ();
> +  if (raise (SIGUSR1))
> +    FAIL_EXIT1 ("error raising signal");
> +  if (do_rseq_main_test ())
> +    result = 1;
> +  for (i = 0; i < N; i++)
> +    {
> +      if (do_rseq_threads_test (t[i]))
> +        result = 1;
> +    }
> +  if (do_rseq_fork_test ())
> +    result = 1;
> +  return result;

OK.

> +}
> +
> +static void
> +atfork_prepare (void)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      printf ("rseq not registered in pthread atfork prepare\n");
> +      support_record_failure ();
> +    }
> +}
> +
> +static void
> +atfork_parent (void)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      printf ("rseq not registered in pthread atfork parent\n");
> +      support_record_failure ();
> +    }
> +}
> +
> +static void
> +atfork_child (void)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      printf ("rseq not registered in pthread atfork child\n");
> +      support_record_failure ();
> +    }
> +}
> +
> +static void
> +rseq_key_destructor (void *arg)
> +{
> +  /* Cannot use deferred failure reporting after main () returns.  */
> +  if (!rseq_thread_registered ())
> +    FAIL_EXIT1 ("rseq not registered in pthread key destructor");
> +}
> +
> +static void
> +do_rseq_create_key (void)
> +{
> +  if (pthread_key_create (&rseq_test_key, rseq_key_destructor))
> +    FAIL_EXIT1 ("error in pthread_key_create");
> +}
> +
> +static void
> +do_rseq_delete_key (void)
> +{
> +  if (pthread_key_delete (rseq_test_key))
> +    FAIL_EXIT1 ("error in pthread_key_delete");
> +}

OK. These two could do with becoming xthread_key_create/xthread_key_delete
wrappers in support/*, but it's not critical.

> +
> +static void
> +atexit_handler (void)
> +{
> +  /* Cannot use deferred failure reporting after main () returns.  */
> +  if (!rseq_thread_registered ())
> +    FAIL_EXIT1 ("rseq not registered in atexit handler");
> +}
> +
> +static void __attribute__ ((constructor))
> +do_rseq_constructor_test (void)
> +{
> +  support_record_failure_init ();
> +  if (atexit (atexit_handler))
> +    {
> +      FAIL_EXIT1 ("error calling atexit");
> +    }
> +  do_rseq_create_key ();
> +  if (pthread_atfork (atfork_prepare, atfork_parent, atfork_child))
> +    FAIL_EXIT1 ("error calling pthread_atfork");
> +  if (do_rseq_test ())
> +    FAIL_EXIT1 ("rseq not registered within constructor");
> +}

OK.

> +
> +static void __attribute__ ((destructor))
> +do_rseq_destructor_test (void)
> +{
> +  /* Cannot use deferred failure reporting after main () returns.  */
> +  if (do_rseq_test ())
> +    FAIL_EXIT1 ("rseq not registered within destructor");
> +  do_rseq_delete_key ();
> +}
> +
> +/* Test C++ destructor called at thread and process exit.  */
> +void
> +__call_tls_dtors (void)
> +{
> +  /* Cannot use deferred failure reporting after main () returns.  */
> +  if (!rseq_thread_registered ())
> +    FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor");
> +}
> +#else
> +static int
> +do_rseq_test (void)
> +{
> +  FAIL_UNSUPPORTED ("kernel headers do not support rseq, skipping test");

OK.

> +  return 0;
> +}
> +#endif
> +
> +static int
> +do_test (void)
> +{
> +  return do_rseq_test ();

OK.

> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/tst-rseq.c b/sysdeps/unix/sysv/linux/tst-rseq.c
> new file mode 100644
> index 0000000000..aaa1ad79fe
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-rseq.c
> @@ -0,0 +1,110 @@

Need a one-line short description.

> +/* Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.

Remove please.

> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* These tests validate that rseq is registered from main in an executable
> +   not linked against libpthread.  */
> +
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +
> +#ifdef __NR_rseq
> +#define HAS_RSEQ
> +#endif
> +
> +#ifdef HAS_RSEQ
> +#include <sys/rseq.h>
> +#include <syscall.h>
> +#include <stdlib.h>
> +#include <error.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +static int
> +rseq_thread_registered (void)
> +{
> +  return (int32_t) __rseq_abi.cpu_id >= 0;
> +}
> +
> +static int
> +do_rseq_main_test (void)
> +{
> +  if (!rseq_thread_registered ())
> +    {
> +      FAIL_RET ("rseq not registered in main thread");
> +    }
> +  return 0;
> +}
> +
> +static int
> +sys_rseq (volatile struct rseq *rseq_abi, uint32_t rseq_len,
> +          int flags, uint32_t sig)
> +{
> +  return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig);
> +}
> +
> +static int
> +rseq_available (void)
> +{
> +  int rc;
> +
> +  rc = sys_rseq (NULL, 0, 0, 0);
> +  if (rc != -1)
> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
> +  switch (errno)
> +    {
> +    case ENOSYS:
> +      return 0;
> +    case EINVAL:
> +      return 1;
> +    default:
> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
> +    }
> +}
> +
> +static int
> +do_rseq_test (void)
> +{
> +  int result = 0;
> +
> +  if (!rseq_available ())
> +    {
> +      FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test");
> +    }
> +  if (do_rseq_main_test ())
> +    result = 1;
> +  return result;
> +}
> +#else
> +static int
> +do_rseq_test (void)
> +{
> +  FAIL_UNSUPPORTED ("kernel headers do not support rseq, skipping test");
> +  return 0;
> +}
> +#endif
> +
> +static int
> +do_test (void)
> +{
> +  return do_rseq_test ();

OK.

> +}
> +
> +#include <support/test-driver.c>
>

----- On Mar 27, 2019, at 5:21 PM, Carlos O'Donell codonell@redhat.com wrote:

> 
>> +
>> +static int
>> +do_rseq_threads_test (int nr_threads)
>> +{
>> +  pthread_t th[nr_threads];
>> +  int i;
>> +  int result = 0;
>> +  pthread_attr_t at;
>> +
>> +  if (pthread_attr_init (&at) != 0)
>> +    {
>> +      FAIL_EXIT1 ("attr_init failed");
>> +    }
>> +
>> +  if (pthread_attr_setstacksize (&at, 1 * 1024 * 1024) != 0)
> 
> Why set it that small? Why set it at all?

I copied that boilerplate code from pre-existing glibc nptl tests,
in a true "monkey see, monkey do" fashion.

If we look at e.g. nptl/tst-fork1.c, those stack size limitations
were introduced by this commit:

commit 4d1a02efc1117763c67fe012642381e3106500cf
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Sun Mar 7 10:40:53 2004 +0000

    Update.

    2004-03-07  Ulrich Drepper  <drepper@redhat.com>
    
            * tst-once4.c: Remove unnecessary macro definition.
    
            * tst-mutex7.c (do_test): Limit thread stack size.
            * tst-once2.c (do_test): Likewise.
            * tst-tls3.c (do_test): Likewise.
            * tst-tls1.c (do_test): Likewise.
            * tst-signal3.c (do_test): Likewise.
            * tst-kill6.c (do_test): Likewise.
            * tst-key4.c (do_test): Likewise.
            * tst-join4.c (do_test): Likewise.
            * tst-fork1.c (do_test): Likewise.
            * tst-context1.c (do_test): Likewise.
            * tst-cond2.c (do_test): Likewise.
            * tst-cond10.c (do_test): Likewise.
            * tst-clock2.c (do_test): Likewise.
            * tst-cancel10.c (do_test): Likewise.
            * tst-basic2.c (do_test): Likewise.
            * tst-barrier4.c (do_test): Likewise.

And that commit does not state _why_ the thread stack size needs to
be limited. Nor on which architectures it matters, and what are the
parameters (e.g. number of threads) for which it matters.

In the case of rseq nptl tests, we spawn up to 50 threads in addition
to main.

Should we limit the thread stack size to 1MB ? That's indeed a good
question. Anyone would like to voice an opinion on the matter ?

>> +static void
>> +do_rseq_create_key (void)
>> +{
>> +  if (pthread_key_create (&rseq_test_key, rseq_key_destructor))
>> +    FAIL_EXIT1 ("error in pthread_key_create");
>> +}
>> +
>> +static void
>> +do_rseq_delete_key (void)
>> +{
>> +  if (pthread_key_delete (rseq_test_key))
>> +    FAIL_EXIT1 ("error in pthread_key_delete");
>> +}
> 
> OK. These two could do with becoming xthread_key_create/xthread_key_delete
> wrappers in support/*, but it's not critical.

I'll add them in a separate commit and modify this commit to use them
instead.

Thanks!

Mathieu

On 4/4/19 4:04 PM, Mathieu Desnoyers wrote:
> ----- On Mar 27, 2019, at 5:21 PM, Carlos O'Donell codonell@redhat.com wrote:
> 
>>
>>> +
>>> +static int
>>> +do_rseq_threads_test (int nr_threads)
>>> +{
>>> +  pthread_t th[nr_threads];
>>> +  int i;
>>> +  int result = 0;
>>> +  pthread_attr_t at;
>>> +
>>> +  if (pthread_attr_init (&at) != 0)
>>> +    {
>>> +      FAIL_EXIT1 ("attr_init failed");
>>> +    }
>>> +
>>> +  if (pthread_attr_setstacksize (&at, 1 * 1024 * 1024) != 0)
>>
>> Why set it that small? Why set it at all?
> 
> I copied that boilerplate code from pre-existing glibc nptl tests,
> in a true "monkey see, monkey do" fashion.
> 
> If we look at e.g. nptl/tst-fork1.c, those stack size limitations
> were introduced by this commit:
> 
> commit 4d1a02efc1117763c67fe012642381e3106500cf
> Author: Ulrich Drepper <drepper@redhat.com>
> Date:   Sun Mar 7 10:40:53 2004 +0000
> 
>      Update.
> 
>      2004-03-07  Ulrich Drepper  <drepper@redhat.com>
>      
>              * tst-once4.c: Remove unnecessary macro definition.
>      
>              * tst-mutex7.c (do_test): Limit thread stack size.
>              * tst-once2.c (do_test): Likewise.
>              * tst-tls3.c (do_test): Likewise.
>              * tst-tls1.c (do_test): Likewise.
>              * tst-signal3.c (do_test): Likewise.
>              * tst-kill6.c (do_test): Likewise.
>              * tst-key4.c (do_test): Likewise.
>              * tst-join4.c (do_test): Likewise.
>              * tst-fork1.c (do_test): Likewise.
>              * tst-context1.c (do_test): Likewise.
>              * tst-cond2.c (do_test): Likewise.
>              * tst-cond10.c (do_test): Likewise.
>              * tst-clock2.c (do_test): Likewise.
>              * tst-cancel10.c (do_test): Likewise.
>              * tst-basic2.c (do_test): Likewise.
>              * tst-barrier4.c (do_test): Likewise.
> 
> And that commit does not state _why_ the thread stack size needs to
> be limited. Nor on which architectures it matters, and what are the
> parameters (e.g. number of threads) for which it matters.
> 
> In the case of rseq nptl tests, we spawn up to 50 threads in addition
> to main.
> 
> Should we limit the thread stack size to 1MB ? That's indeed a good
> question. Anyone would like to voice an opinion on the matter ?

Unless you have a strong reason for selecting 1MiB you should just
remove the boiler plate code and let the architecture default stack
sizes apply.

The above commit is a good example of a failure to provide a comment
that gives intent for the implementation and therefore you have no
idea why 1MiB was selected. Magic numbers should have comments, and
a patch like the one you reference would not be accepted today.

The only real worry we have with testing is thread reap rate which
seems to be slow in the kernel and sometimes we've seen the kernel
be unable to clone new threads because of this reason. Even then on
the worst architecture, hppa, I can create ~300 threads in a test
without any problems.

In summary:
- Remove the thread stack size setting.
- Optional: Send a distinct patch to cleanup the tests which have
   such boiler plate ;-)

----- On Apr 4, 2019, at 4:11 PM, Carlos O'Donell codonell@redhat.com wrote:

> On 4/4/19 4:04 PM, Mathieu Desnoyers wrote:
>> ----- On Mar 27, 2019, at 5:21 PM, Carlos O'Donell codonell@redhat.com wrote:
>> 
>>>
>>>> +
>>>> +static int
>>>> +do_rseq_threads_test (int nr_threads)
>>>> +{
>>>> +  pthread_t th[nr_threads];
>>>> +  int i;
>>>> +  int result = 0;
>>>> +  pthread_attr_t at;
>>>> +
>>>> +  if (pthread_attr_init (&at) != 0)
>>>> +    {
>>>> +      FAIL_EXIT1 ("attr_init failed");
>>>> +    }
>>>> +
>>>> +  if (pthread_attr_setstacksize (&at, 1 * 1024 * 1024) != 0)
>>>
>>> Why set it that small? Why set it at all?
>> 
>> I copied that boilerplate code from pre-existing glibc nptl tests,
>> in a true "monkey see, monkey do" fashion.
>> 
>> If we look at e.g. nptl/tst-fork1.c, those stack size limitations
>> were introduced by this commit:
>> 
>> commit 4d1a02efc1117763c67fe012642381e3106500cf
>> Author: Ulrich Drepper <drepper@redhat.com>
>> Date:   Sun Mar 7 10:40:53 2004 +0000
>> 
>>      Update.
>> 
>>      2004-03-07  Ulrich Drepper  <drepper@redhat.com>
>>      
>>              * tst-once4.c: Remove unnecessary macro definition.
>>      
>>              * tst-mutex7.c (do_test): Limit thread stack size.
>>              * tst-once2.c (do_test): Likewise.
>>              * tst-tls3.c (do_test): Likewise.
>>              * tst-tls1.c (do_test): Likewise.
>>              * tst-signal3.c (do_test): Likewise.
>>              * tst-kill6.c (do_test): Likewise.
>>              * tst-key4.c (do_test): Likewise.
>>              * tst-join4.c (do_test): Likewise.
>>              * tst-fork1.c (do_test): Likewise.
>>              * tst-context1.c (do_test): Likewise.
>>              * tst-cond2.c (do_test): Likewise.
>>              * tst-cond10.c (do_test): Likewise.
>>              * tst-clock2.c (do_test): Likewise.
>>              * tst-cancel10.c (do_test): Likewise.
>>              * tst-basic2.c (do_test): Likewise.
>>              * tst-barrier4.c (do_test): Likewise.
>> 
>> And that commit does not state _why_ the thread stack size needs to
>> be limited. Nor on which architectures it matters, and what are the
>> parameters (e.g. number of threads) for which it matters.
>> 
>> In the case of rseq nptl tests, we spawn up to 50 threads in addition
>> to main.
>> 
>> Should we limit the thread stack size to 1MB ? That's indeed a good
>> question. Anyone would like to voice an opinion on the matter ?
> 
> Unless you have a strong reason for selecting 1MiB you should just
> remove the boiler plate code and let the architecture default stack
> sizes apply.
> 
> The above commit is a good example of a failure to provide a comment
> that gives intent for the implementation and therefore you have no
> idea why 1MiB was selected. Magic numbers should have comments, and
> a patch like the one you reference would not be accepted today.
> 
> The only real worry we have with testing is thread reap rate which
> seems to be slow in the kernel and sometimes we've seen the kernel
> be unable to clone new threads because of this reason. Even then on
> the worst architecture, hppa, I can create ~300 threads in a test
> without any problems.
> 
> In summary:
> - Remove the thread stack size setting.
> - Optional: Send a distinct patch to cleanup the tests which have
>   such boiler plate ;-)

I'll do the 1st item, and let the 2nd one to another brave soul. ;)

Thanks,

Mathieu

> 
> --
> Cheers,
> Carlos.

* Carlos O'Donell:

> The above commit is a good example of a failure to provide a comment
> that gives intent for the implementation and therefore you have no
> idea why 1MiB was selected. Magic numbers should have comments, and
> a patch like the one you reference would not be accepted today.
>
> The only real worry we have with testing is thread reap rate which
> seems to be slow in the kernel and sometimes we've seen the kernel
> be unable to clone new threads because of this reason. Even then on
> the worst architecture, hppa, I can create ~300 threads in a test
> without any problems.

Delayed reaping in the kernel (after signaling thread exit) does *not*
affect the stack allocation.  With a valid test, the stack is queued for
reuse.  Only kernel-side data structures stick around.

My guess is that in 2004, 64-bit systems were still around that didn't
have enough physical backing store for 50 * 8 MiB thread stacks, so the
overcommit limiter in the kernel would kick in.  I don't think this is a
problem anymore, and in the off chance that it is, you can still use
ulimit -s to reduce the default if necessary.

Thanks,
Florian

On 4/5/19 6:01 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> The above commit is a good example of a failure to provide a comment
>> that gives intent for the implementation and therefore you have no
>> idea why 1MiB was selected. Magic numbers should have comments, and
>> a patch like the one you reference would not be accepted today.
>>
>> The only real worry we have with testing is thread reap rate which
>> seems to be slow in the kernel and sometimes we've seen the kernel
>> be unable to clone new threads because of this reason. Even then on
>> the worst architecture, hppa, I can create ~300 threads in a test
>> without any problems.
> 
> Delayed reaping in the kernel (after signaling thread exit) does *not*
> affect the stack allocation.  With a valid test, the stack is queued for
> reuse.  Only kernel-side data structures stick around.

Unless you run out of mappings? The kernel must handle CLONE_CHILD_CLEARTID
in a timely fashion or glibc will be unable to free the stacks and the cache
could grow beyond the maximum limit (note that free_stacks() is only a
one-shot attempt to lower the limit and does not need to succeed).

> My guess is that in 2004, 64-bit systems were still around that didn't
> have enough physical backing store for 50 * 8 MiB thread stacks, so the
> overcommit limiter in the kernel would kick in.  I don't think this is a
> problem anymore, and in the off chance that it is, you can still use
> ulimit -s to reduce the default if necessary.

Agreed.

* Carlos O'Donell:

> On 4/5/19 6:01 AM, Florian Weimer wrote:
>> * Carlos O'Donell:
>>
>>> The above commit is a good example of a failure to provide a comment
>>> that gives intent for the implementation and therefore you have no
>>> idea why 1MiB was selected. Magic numbers should have comments, and
>>> a patch like the one you reference would not be accepted today.
>>>
>>> The only real worry we have with testing is thread reap rate which
>>> seems to be slow in the kernel and sometimes we've seen the kernel
>>> be unable to clone new threads because of this reason. Even then on
>>> the worst architecture, hppa, I can create ~300 threads in a test
>>> without any problems.
>>
>> Delayed reaping in the kernel (after signaling thread exit) does *not*
>> affect the stack allocation.  With a valid test, the stack is queued for
>> reuse.  Only kernel-side data structures stick around.
>
> Unless you run out of mappings? The kernel must handle CLONE_CHILD_CLEARTID
> in a timely fashion or glibc will be unable to free the stacks and the cache
> could grow beyond the maximum limit (note that free_stacks() is only a
> one-shot attempt to lower the limit and does not need to succeed).

The reaping problem is that we get CLONE_CHILD_CLEARTID notification
(and the application spawns a new thread) before the old thread is
completely gone.  It's no longer running, so we can safely remove the
stack, but not all kernel data structures have been deallocated at that
point.

Or do we have tests that spawn detached threads in a loop, expecting not
to exceed the thread/task limit of the user?  That's a different problem
and probably an invalid test.

Thanks,
Florian

On 4/5/19 11:27 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 4/5/19 6:01 AM, Florian Weimer wrote:
>>> * Carlos O'Donell:
>>>
>>>> The above commit is a good example of a failure to provide a comment
>>>> that gives intent for the implementation and therefore you have no
>>>> idea why 1MiB was selected. Magic numbers should have comments, and
>>>> a patch like the one you reference would not be accepted today.
>>>>
>>>> The only real worry we have with testing is thread reap rate which
>>>> seems to be slow in the kernel and sometimes we've seen the kernel
>>>> be unable to clone new threads because of this reason. Even then on
>>>> the worst architecture, hppa, I can create ~300 threads in a test
>>>> without any problems.
>>>
>>> Delayed reaping in the kernel (after signaling thread exit) does *not*
>>> affect the stack allocation.  With a valid test, the stack is queued for
>>> reuse.  Only kernel-side data structures stick around.
>>
>> Unless you run out of mappings? The kernel must handle CLONE_CHILD_CLEARTID
>> in a timely fashion or glibc will be unable to free the stacks and the cache
>> could grow beyond the maximum limit (note that free_stacks() is only a
>> one-shot attempt to lower the limit and does not need to succeed).
> 
> The reaping problem is that we get CLONE_CHILD_CLEARTID notification
> (and the application spawns a new thread) before the old thread is
> completely gone.  It's no longer running, so we can safely remove the
> stack, but not all kernel data structures have been deallocated at that
> point.

A call to pthread_join does not re-evaluate the stack cache limits and does
not free anything from the cache.

Therefore you can have hundreds of threads exit, go through free_stacks(),
fail to free their stacks, and *then* hit pthread_join(), still fail to
free any stacks (because we don't re-reap the stacks there, is that our
fault in our __deallocate_stack() impl?), and then try to do some other
operation that requires memory and run out.

> Or do we have tests that spawn detached threads in a loop, expecting not
> to exceed the thread/task limit of the user?  That's a different problem
> and probably an invalid test.

No, that only happens in out 4 test cases and doesn't involve many threads.

* Carlos O'Donell:

> A call to pthread_join does not re-evaluate the stack cache limits and does
> not free anything from the cache.

Are you sure?  I assumed that we have this call stack:

  pthread_join
  __pthread_timedjoin_ex
  __free_tcb
  __deallocate_stack
  queue_stack
  free_stacks

And since we call __free_tcb only after the futex wait on the TID
completes, free_stacks observe the stack of the just-joined thread as
unused.

(We should probably trim the Cc: list at this point, sorry.)

Thanks,
Florian

On 4/5/19 3:43 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> A call to pthread_join does not re-evaluate the stack cache limits and does
>> not free anything from the cache.
> 
> Are you sure?  I assumed that we have this call stack:
> 
>    pthread_join
>    __pthread_timedjoin_ex
>    __free_tcb
>    __deallocate_stack
>    queue_stack
>    free_stacks
> 
> And since we call __free_tcb only after the futex wait on the TID
> completes, free_stacks observe the stack of the just-joined thread as
> unused.

(CC list trimmed)

You are correct. Thanks for noting the call to queue_stacks in
__deallocate_stack, I missed that, I thought we added directly
to the cache.