Re: [PATCH 4/4] rseq registration tests (v2)

[Mathieu Desnoyers <mathieu dot desnoyers at efficios dot com>]

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

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