RFR(xxs): 8187230: [aix] Leave OS guard page size at default for non-java threads instead of explicitly setting it

[David Holmes]

Looks fine to me.

Cheers,
David

On 18/10/2017 5:10 PM, Thomas Stüfe wrote:
> Hi all,
> 
> I am cleaning up my backlog of old issues which did not make it into the 
> repo before the consolidation.
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8187230 
> <https://bugs.openjdk.java.net/browse/JDK-8187230>
> 
> Last Webrev (just rebased to the new repo structure, no changes):
> http://cr.openjdk.java.net/~stuefe/webrevs/8187230-aix-leave-os-guard-page-size-at-default-for-non-java-threads/webrev.02/webrev/
> 
> For your convenience, here the original message:
> 
> <<<
> The change is very subtle.
> 
> Before, we would set the OS guard page size for every thread - for java 
> threads disable them, for non-java threads we'd set them to 4K.
> 
> Now, we still disable them for java threads but leave them at the OS 
> default size for non-java threads.
> 
> The really important part is the disabling of OS guard pages for java 
> threads, where we have a VM guard pages in place and do not want to 
> spend more memory on OS guards. We do not really care for the exact size 
> of the OS guard pages for non-java threads, and therefore should not set 
> it - we should leave the size in place the OS deems sufficient. That 
> also spares us the complexity of handling the thread stack page size, 
> which on AIX may be different from os::vm_page_size().
>  >>>
> 
> @Chris: you did ask whether this would make sense for Linux too. I think 
> you are right, but as Goetz pointed out matters are more complicated as 
> glibc pthread_create does not substract OS guard size from the user 
> specified stack size, so it requires us to know the OS guard size and 
> add it to the specified stack size (funny, the same issue we have with 
> VM guards and -Xss). So, for now, I'd prefer this to keep AIX only.
> 
> I think I need a second reviewer beside Goetz.
> 
> Thanks!
> 
> Thomas
> 
> 
> 
> On Fri, Sep 8, 2017 at 10:48 AM, Thomas Stüfe <thomas.stuefe at gmail.com 
> <mailto:thomas.stuefe at gmail.com>> wrote:
> 
>     Hi Guys,
> 
>     On Fri, Sep 8, 2017 at 9:51 AM, Lindenmaier, Goetz
>     <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com>> wrote:
> 
>         Hi Chris,
> 
>         on linux the pthread implementation is a bit strange, or buggy.
>         It takes the OS guard pages out of the stack size specified.
>         We need to set it so we can predict the additional space
>         that must be allocated for the stack.
> 
>         See also the comment in os_linux.cpp, create_thread().
> 
> 
>     Goetz, I know we talked about this off list yesterday, but now I am
>     not sure this is actually needed. Yes, to correctly calculate the
>     stack size, we need to know the OS guard page size, but we do not
>     need to set it, we just need to know it. So, for non-java threads
>     (java threads get the OS guard set to zero), it would probably be
>     sufficient to:
> 
>     - pthread_attr_init() (sets default thread attribute values to the
>     attribute structure) and then
>     - pthread_attr_getguardsize() to read the guard size from that
>     structure.
> 
>     That way we leave the OS guard page at the size glibc deems best. I
>     think that is a better option. Consider a situation where the glibc
>     changes the size of the OS guard pages, for whatever reason - we
>     probably should follow suit.
> 
>     See e.g. this security issue - admittedly only loosely related,
>     since the fix for this issue seemed to be a fix to stack banging,
>     not changing the OS guard size:
>     https://access.redhat.com/security/vulnerabilities/stackguard
>     <https://access.redhat.com/security/vulnerabilities/stackguard>
> 
>     So, in short, I think we could change this for Linux too. If you
>     guys agree, I'll add this to the patch. Since I am on vacation and
>     the depot is closed, it may take some time.
> 
>     Kind Regards, Thomas
> 
> 
> 
> 
>         Best regards,
>            Goetz.
> 
>         > -----Original Message-----
>         > From: ppc-aix-port-dev [mailto:ppc-aix-port-dev-bounces at openjdk.java.net
>         <mailto:ppc-aix-port-dev-bounces at openjdk.java.net>]
>         > On Behalf Of Chris Plummer
>         > Sent: Thursday, September 07, 2017 11:07 PM
>         > To: Thomas Stüfe <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>;
>         ppc-aix-port-
>         > dev at openjdk.java.net <mailto:dev at openjdk.java.net>
>         > Cc: HotSpot Open Source Developers <hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>>
>         > Subject: Re: RFR(xxs): 8187230: [aix] Leave OS guard page size at default for
>         > non-java threads instead of explicitly setting it
>         >
>          > Hi Thomas,
>          >
>          > Is there a reason this shouldn't also be done for linux?
>          >
>          > thanks,
>          >
>          > Chris
>          >
>          > On 9/7/17 3:02 AM, Thomas Stüfe wrote:
>          > > Hi all,
>          > >
>          > > may I please have a review for this small change:
>          > >
>          > > Bug:
>          > > https://bugs.openjdk.java.net/browse/JDK-8187230
>         <https://bugs.openjdk.java.net/browse/JDK-8187230>
>          > >
>          > > Webrev:
>          > > http://cr.openjdk.java.net/~stuefe/webrevs/8187230-aix-
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8187230-aix->
>          > > leave-os-guard-page-size-at-default-for-non-java-
>          > threads/webrev.00/webrev/
>          > >
>          > > The change is very subtle.
>          > >
>          > > Before, we would set the OS guard page size for every
>         thread - for java
>          > > threads disable them, for non-java threads we'd set them to 4K.
>          > >
>          > > Now, we still disable them for java threads but leave them
>         at the OS
>          > > default size for non-java threads.
>          > >
>          > > The really important part is the disabling of OS guard
>         pages for java
>          > > threads, where we have a VM guard pages in place and do not
>         want to
>          > spend
>          > > more memory on OS guards. We do not really care for the
>         exact size of the
>          > > OS guard pages for non-java threads, and therefore should
>         not set it - we
>          > > should leave the size in place the OS deems sufficient.
>         That also spares us
>          > > the complexity of handling the thread stack page size,
>         which on AIX may be
>          > > different from os::vm_page_size().
>          > >
>          > > Thank you and Kind Regards, Thomas
>          >
>          >
> 
> 
>