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

[Thomas Stuefe]

On Fri, 21 Apr 2023 04:57:28 GMT, David Holmes <dholmes 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

Looks mostly okay. I had to compare the glibc sources for 2.26 and 2.27 in order to understand what the patch does.

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?

src/hotspot/os/linux/os_linux.cpp line 843:

> 841:     // then we need to perform the adjustment.
> 842:     os::Linux::AdjustStackSizeForGuardPages = (min_stack2 - min_stack > 0);
> 843:     log_info(os)("- glibc stack size guard page adjustment is %sneeded",

Why the leading dash?

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?

src/hotspot/os/linux/os_linux.cpp line 4549:

> 4547: 
> 4548:   // Check if we need to adjust the stack size for glibc guard pages.
> 4549:   init_adjust_stack_for_guard_pages();

Please make this glibc only. I agree with @TheRealMDoerr, muslc does not have `__pthread_get_minstack`, no need to pay for a dynamic lookup that will always fail.

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.

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

PR Review: https://git.openjdk.org/jdk/pull/13571#pullrequestreview-1396732426
PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174410930
PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174420872
PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174418676
PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174419119
PR Review Comment: https://git.openjdk.org/jdk/pull/13571#discussion_r1174418172