This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH 4/4] rseq registration tests (v2)
- From: Carlos O'Donell <codonell at redhat dot com>
- To: Mathieu Desnoyers <mathieu dot desnoyers at efficios dot com>
- Cc: carlos <carlos at redhat dot com>, Florian Weimer <fweimer at redhat dot com>, Joseph Myers <joseph at codesourcery dot com>, Szabolcs Nagy <szabolcs dot nagy at arm dot com>, libc-alpha <libc-alpha at sourceware dot org>, Thomas Gleixner <tglx at linutronix dot de>, Ben Maurer <bmaurer at fb dot com>, Peter Zijlstra <peterz at infradead dot org>, "Paul E. McKenney" <paulmck at linux dot vnet dot ibm dot com>, Boqun Feng <boqun dot feng at gmail dot com>, Will Deacon <will dot deacon at arm dot com>, Dave Watson <davejwatson at fb dot com>, Paul Turner <pjt at google dot com>
- Date: Thu, 4 Apr 2019 16:11:09 -0400
- Subject: Re: [PATCH 4/4] rseq registration tests (v2)
- References: <firstname.lastname@example.org> <email@example.com> <firstname.lastname@example.org> <1950762361.2578.1554408264573.JavaMail.email@example.com>
On 4/4/19 4:04 PM, Mathieu Desnoyers wrote:
----- On Mar 27, 2019, at 5:21 PM, Carlos O'Donell firstname.lastname@example.org wrote:
+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:
Author: Ulrich Drepper <email@example.com>
Date: Sun Mar 7 10:40:53 2004 +0000
2004-03-07 Ulrich Drepper <firstname.lastname@example.org>
* 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
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
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.
- Remove the thread stack size setting.
- Optional: Send a distinct patch to cleanup the tests which have
such boiler plate ;-)