Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Apr 21, 2023

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

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

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

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13571/head:pull/13571
$ git checkout pull/13571

Update a local copy of the PR:
$ git checkout pull/13571
$ git pull https://git.openjdk.org/jdk.git pull/13571/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13571

View PR using the GUI difftool:
$ git pr show -t 13571

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13571.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 21, 2023

👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 21, 2023
@openjdk
Copy link

openjdk bot commented Apr 21, 2023

@dholmes-ora The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Apr 21, 2023
@mlbridge
Copy link

mlbridge bot commented Apr 21, 2023

Webrevs

@TheRealMDoerr
Copy link
Contributor

Hi David, does this work for Alpine or should it be glibc only?

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, despite the runtime check. AFAIU, this does not affect directly affect RSS, because we don't commit guard pages?

src/hotspot/os/linux/os_linux.hpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
pthread_attr_destroy(&attr);
// If the minimum stack size changed when we added the guard pages
// then we need to perform the adjustment.
os::Linux::AdjustStackSizeForGuardPages = (min_stack2 - min_stack > 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subtracting the unsigned size_t is somewhat unsafe in face of underflows here. Just min_stack2 > min_stack solves it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I guess minstack2 != minstack suffices in that regard, but underflow is not a consideration here surely? If the difference was more than the specified guard size (modulo rounding) then there is a more serious glibc bug here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just != is fine as well. - would raise some eyebrows with static analyzers, I would have thought.

@dholmes-ora
Copy link
Member Author

Hi David, does this work for Alpine or should it be glibc only?

The existing code applies the workaround regardless of libc implementation. I assume musl C library would not have __pthread_get_minstack in which case the new code will preserve the existing behaviour and apply the adjustment. I've no idea whether musl C library handles guard pages correctly or not.

@dholmes-ora
Copy link
Member Author

I like this, despite the runtime check. AFAIU, this does not affect directly affect RSS, because we don't commit guard pages?

I don't know the answer to that sorry.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/os_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.hpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
@dholmes-ora
Copy link
Member Author

Before I forget, Thanks for looking at this @shipilev , @TheRealMDoerr and @tstuefe :)

@tstuefe
Copy link
Member

tstuefe commented Apr 24, 2023

@tstuefe that is all true and correct, but what we don't know (without looking at the implementation) is what __get_minstack will do with the value in the attribute. I guess I need to find the old version of the code and see whether it does a raw read or adjusts it somehow based on pagesize.

HEAD

size_t
__pthread_get_minstack (const pthread_attr_t *attr)
{
  return (GLRO(dl_pagesize) + __nptl_tls_static_size_for_stack ()
          + PTHREAD_STACK_MIN);
}

2.27

size_t
__pthread_get_minstack (const pthread_attr_t *attr)
{
  return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN;
} 

2.26

size_t
__pthread_get_minstack (const pthread_attr_t *attr)                                                                                                                                                                                                                             
{
  struct pthread_attr *iattr = (struct pthread_attr *) attr;

  return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN
          + iattr->guardsize);
}

Old version just added the guard size, without alignment. So your trick should work.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve, patch looks like it would work and you seem to want to get it off your table quickly. I don't want to keep this up - any cleanups can be done in separate RFEs, if you prefer.

Cheers, Thomas

@openjdk
Copy link

openjdk bot commented Apr 24, 2023

@dholmes-ora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: shade, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 92 new commits pushed to the master branch:

  • 86f41a4: 8306735: G1: G1FullGCScope remove unnecessary member _explicit_gc
  • d747698: 8306823: Native memory leak in SharedRuntime::notify_jvmti_unmount/mount.
  • 8d89992: 8298189: Regression in SPECjvm2008-MonteCarlo for pre-Cascade Lake Intel processors
  • 44d9f55: 8306072: Open source several AWT MouseInfo related tests
  • cc894d8: 8303466: C2: failed: malformed control flow. Limit type made precise with MaxL/MinL
  • ed1ebd2: 8306652: Open source AWT MenuItem related tests
  • f3e8bd1: 8306755: Open source few Swing JComponent and AbstractButton tests
  • 1c1a73f: 8302908: RISC-V: Support masked vector arithmetic instructions for Vector API
  • adf62fe: 8304918: Remove unused decl field from AnnotatedType implementations
  • 00b1eac: 8306031: Update IANA Language Subtag Registry to Version 2023-04-13
  • ... and 82 more: https://git.openjdk.org/jdk/compare/64ed816ad9f1a9773c9865a013e89b709a130e9c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 24, 2023
@dholmes-ora
Copy link
Member Author

Sorry for the delay - I had an extra long weekend. I think I have now addressed all of the concerns raised. Thanks.

@tstuefe
Copy link
Member

tstuefe commented Apr 26, 2023

Still good. Thanks for taking in my input.

About "stack size does not matter", the sources I looked at were the upstream glibc sources. I did not check any downstream vendor ones, e.g. if IBM changed the logic on 64k paged ppc glibc. But if that is a problem, it can be fixed later.

@dholmes-ora
Copy link
Member Author

Thanks @tstuefe !

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Checking myself here: the default is still the same: "adjust", and this patch disables that optionally if we can prove the adjustment is not need with a runtime check.

@dholmes-ora
Copy link
Member Author

Checking myself here: the default is still the same: "adjust", and this patch disables that optionally if we can prove the adjustment is not need with a runtime check.

Correct. Thanks for the review @shipilev !

/integrate

@openjdk
Copy link

openjdk bot commented Apr 26, 2023

Going to push as commit 9ebcda2.
Since your change was applied there have been 107 commits pushed to the master branch:

  • 1e4eafb: 8071693: Introspector ignores default interface methods
  • 750bece: 8305771: SA ClassWriter.java fails to skip overpass methods
  • b81c9c8: 8306951: [BACKOUT] JDK-8305252 make_method_handle_intrinsic may call java code under a lock
  • 732179c: 8306409: Open source AWT KeyBoardFocusManger, LightWeightComponent related tests
  • 38cc039: 8306705: com/sun/jdi/PopAndInvokeTest.java fails with NativeMethodException
  • 01b8512: 8302182: Update Public Suffix List to 88467c9
  • 8e36c05: 8305853: java/text/Format/DateFormat/DateFormatRegression.java fails with "Uncaught exception thrown in test method Test4089106"
  • d0e8aec: 8306374: (bf) Improve performance of DirectCharBuffer::append(CharSequence[,int,int])
  • a18191f: 8302328: [s390x] Simplify asm_assert definition
  • 9bc6a21: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
  • ... and 97 more: https://git.openjdk.org/jdk/compare/64ed816ad9f1a9773c9865a013e89b709a130e9c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 26, 2023
@openjdk openjdk bot closed this Apr 26, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 26, 2023
@openjdk
Copy link

openjdk bot commented Apr 26, 2023

@dholmes-ora Pushed as commit 9ebcda2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@dholmes-ora dholmes-ora deleted the 8229147-guardpage branch April 26, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants