Re: [PATCH] Fix nptl/tst-cond1{6,7} on 32-bit with many cpus

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Tue, Mar 27, 2012 at 3:09 PM, Chris Metcalf <cmetcalf@tilera.com> wrote:
> On 3/27/2012 2:45 PM, Carlos O'Donell wrote:
>> On Mon, Mar 26, 2012 at 5:34 PM, David Miller <davem@davemloft.net> wrote:
>>> From: "Carlos O'Donell" <carlos@systemhalted.org>
>>> Date: Mon, 26 Mar 2012 10:44:21 -0400
>>>> On Sun, Mar 25, 2012 at 9:47 PM, David Miller <davem@davemloft.net> wrote:
>>>>> @@ -76,9 +76,15 @@ do_test (void)
>>>>> ? count *= 4;
>>>>>
>>>>> ? pthread_t th[count];
>>>>> - ?int i, ret;
>>>>> + ?pthread_attr_t attr;
>>>>> + ?int i, ret, sz;
>>>>> + ?pthread_attr_init (&attr);
>>>>> + ?sz = __getpagesize ();
>>>>> + ?if (sz < 64 * 1024)
>>>>> + ? ? ? ? sz = 64 * 1024;
>>>> Should this be PTHREAD_STACK_MIN instead to allow for per-machine
>>>> variations? Is it sufficient to use PTHREAD_STACK_MIN?
>>> We still need to take __getpagesize() into account, because some
>>> PTHREAD_STACK_MIN definitions are smaller than the largest possible
>>> page size on the respective architecture.
>> This looks good to me, please check this in.
>>
>> The pedant inside of me says that this is wrong and that
>> PTHREAD_STACK_MIN should be sufficient. Unforutnately the more
>> fundamental problem is that glibc treats stack as "stack +
>> implementation required space" and renders PTHREAD_STACK_MIN somewhat
>> broken in light of variable sized static TLS data. I'm going to
>> approach the Austin Group to get an answer for the question of "What
>> is stack?"
>>
>> I'd be surprised if PTHREAD_STACK_MIN even works correctly in such
>> cases (less than a page) since you need a minimum size for a guard
>> (page aligned), MINIMAL_REST_STACK, and __static_tls_Size.
>
> Nonetheless, I think a check of PTHREAD_STACK_MIN is required, since if the
> pthread_attr_setstacksize() value is for less than that, it's going to get
> ignored with EINVAL. ?On tilegx with the default 64K page size, the min
> stack size is 128K (for reasons that escape me at the moment), so you need
> to have the stack at least that big. ?We could either add "if (sz <
> PTHREAD_STACK_MIN) sz = PTHREAD_STACK_MIN;" or else just use
> PTHREAD_STACK_MIN instead of 64 * 1024.

I agree that the check for PTHREAD_STACK_MIN is required.

What I would like to see is the check go away entirely and just set
the stack size to PTHREAD_STACK_MIN without doing any checking. It
should always be OK to use PTHREAD_STACK_MIN as the minimum when you
have a thread that doesn't do very much, and it should work in all
reasonable cases (like in this test).

However, as Dave points out PTHREAD_STACK_MIN is a static value that
might, given the kernel configuration, be smaller than a page, in
which case Dave wants it to be a full page instead of
PTHREAD_STACK_MIN. This makes the code more robust given the *current*
implementation of nptl/allocatestack.c(allocate_stack).

For example in allocate_stack() it would appear that even using
PTHREAD_STACK_MIN you'll get back EINVAL if the kernel's returned
pagesize via AT_PAGESIZE is > PTHREAD_STACK_MIN. Why? Because the
guard page (forcibly rounded to a page) will consume the entire size.

Again, this is another strong reason for fixing BZ#11787
(http://sourceware.org/bugzilla/show_bug.cgi?id=11787) such that the
stack size doesn't include the guard or other implementation defined
pieces.

Cheers,
Carlos.