Re: [PATCH][BZ #11787] Fix stack guard size accounting

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

On 08/01/18 15:22, Florian Weimer wrote:
> On 01/08/2018 04:20 PM, Carlos O'Donell wrote:
>> On 12/12/2017 06:21 AM, Szabolcs Nagy wrote:
>>> Previously if user requested S stack and G guard when creating a
>>> thread, the total mapping was S and the actual available stack was
>>> S - G - static_tls, which is not what the user requested.
>>>
>>> This patch fixes the guard size accounting by pretending the user
>>> requested S + G stack.  This way all later logic works out except
>>> when reporting the user requested stack size (pthread_getattr_np)
>>> or when computing the minimal stack size (__pthread_get_minstack).
>>>
>>> Normally this will increase thread stack allocations by one page.
>>> TLS accounting is not affected, that will require a separate fix.
>>>
>>> 2017-12-12  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>>
>>>     [BZ #11787]
>>>     * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize.
>>>     * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from
>>>     stacksize.
>>>     * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise.
>>>
>>
>> Any status on this?
>>
>> I would like to get this in as soon as possible because the current
>> Intel fxsave/xsave/xsavec consume more stack than before, and we have
>> had at least one report of an application failing because of the additional
>> stack usage (breaks ntpd helper threads running with PTHREAD_STACK_MIN, see
>> the other discussion about what good is PTHREADS_STACK_MIN).
>>
>> It looks like a next step would be:
>>
>> * Split into two patches.
>> * First patch is as you intended (and can be checked in right away)
> 
> Agreed.
> 

sorry i did not work on this while on holiday
will submit an updated patch

>> * Second patch removed the unused pthread_attr_r and renames __pthread_get_minstack
>>    to avoid usage by other programs.
> 
> The proposed second patch *still* breaks Rust programs with large static TLS data because it will switch to
> PTHREAD_STACK_MIN.  We should do this only after __pthread_get_minstack turns constant (that is, returns
> PTHREAD_STACK_MIN).

i knew __pthread_get_minstack was used outside of glibc
but thought removing the guardsize was safe since we
add it back later.

the patch increases stack allocations by +guardsize,
so if user computes its stack via __pthread_get_minstack
that may now return a smaller value to get the same stack
allocation as before in the end.