RFR: 8229147: Linux os::create_thread() overcounts guardpage size with newer glibc (>=2.27)

[David Holmes]

On Sat, 22 Apr 2023 12:53:32 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> We can now detect whether glibc includes the guard pages as part of the requested stack size or not, and so only need to make adjustments when glibc requires it.
>> 
>> The intent was to use a local variable as the "flag" but unfortunately it is also needed in os_posix.cpp so I had to make it part of the os::Linux API.
>> 
>> See bug report (and related) for details.
>> 
>> Testing:
>>   - Manually checked log output for stack sizes and boundaries on systems with and without the glibc fix. (Again see JBS issue)
>>   -  Tiers 1-3 sanity
>> Thanks
>
> src/hotspot/os/linux/os_linux.cpp line 773:
> 
>> 771: 
>> 772: static void get_minstack_init() {
>> 773:   if (_get_minstack_func == nullptr) {
> 
> We should only ever call this once, so I why not assert == nullptr here instead of the null check?

Right - earlier there were two potential initialization paths but now there is only one.

> src/hotspot/os/linux/os_linux.cpp line 1029:
> 
>> 1027:                        ", stack: " PTR_FORMAT " - " PTR_FORMAT " (" SIZE_FORMAT "k) ).",
>> 1028:                        os::current_thread_id(), (uintx) pthread_self(),
>> 1029:                        p2i(thread->stack_base()), p2i(thread->stack_end()), thread->stack_size() / K);
> 
> Separate fix maybe, to make back porting simpler?

How does this have any affect on backporting?

> src/hotspot/os/posix/os_posix.cpp line 909:
> 
>> 907:   pthread_attr_getguardsize(attr, &guard_size);
>> 908:   // Work around glibc stack guard issue, see os::create_thread() in os_linux.cpp.
>> 909:   LINUX_ONLY(if (os::Linux::AdjustStackSizeForGuardPages) stack_size -= guard_size;)
> 
> Preexisting, but I am actually not sure adjusting the value here is useful; the original code was meant to be a debugging aid for developers and should show the actual value set in the attribute, not second-guess what some libc maybe would do with it. I see Goetz changed this with JDK-8169373. 
> 
> If we get rid of this adjustment and just display the value as-is, we avoid having to export AdjustStackSizeForGuardPages and can remove the ugly platform ifdef.

That is a different issue. It was decided "back then" that this was in fact the desired behaviour - to report the usable stacksize not the raw stack size.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174491067
PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174491283
PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174491228