RFR(S): 8186286: [BSD] Primary thread's stack size is reported incorrectly

[Haug, Gunter]

Hi David,

Sorry for the late reply! I have corrected the typos and updated the change, here is the new webrev:

http://cr.openjdk.java.net/~ghaug/webrevs/8186286.v2

Would you mind sponsoring it?

Re not using os::vm_page_size(): You’re right, and I thought the same - it’s certainly correct for the primary thread. However, os::vm_page_size() isn’t used in the function and I think it’s not much of a problem to call getpagesize() in this particular case.

Thanks and best regards,
Gunter



On 18.08.17, 06:18, "David Holmes" <david.holmes at oracle.com> wrote:

    Hi Thomas,
    
    On 18/08/2017 1:59 PM, Thomas Stüfe wrote:
    > Hi David,
    > 
    > On Thu, Aug 17, 2017 at 11:54 PM, David Holmes <david.holmes at oracle.com 
    > <mailto:david.holmes at oracle.com>> wrote:
    > 
    >     Hi Gunter,
    > 
    >     On 18/08/2017 1:01 AM, Haug, Gunter wrote:
    > 
    >         Thanks for the review, Aleksey and Thomas
    > 
    >         You’re right, it is much nicer to use the macros. I’ve updated
    >         the change accordingly:
    >         http://cr.openjdk.java.net/~ghaug/webrevs/8186286.v1
    >         <http://cr.openjdk.java.net/~ghaug/webrevs/8186286.v1>
    > 
    > 
    >     I'm unclear about the logic change here. If you round down then
    >     there is a chance you will enter the following if-block where
    >     otherwise you would not.
    > 
    >       915     *size = align_down(*size, getpagesize());
    >       916
    >       917     if ((*size) < (DEFAULT_MAIN_THREAD_STACK_PAGES *
    >     (size_t)getpagesize())) {
    > 
    > 
    > I don't see how this could happen? Gunters fix causes the size to snap 
    > to the lower page boundary, but never cross it. The code below ensures a 
    > lower cap at a page boundary.
    
    Yes you are right - sorry for the noise.
    
    David
    
    > If size was larger than that cap, after the align_down it will be at 
    > most right at that cap. If it was lower than the cap, it stays lower. In 
    > both cases behavior  is unchanged.
    > 
    > Cheers, Thomas
    > 
    >     Have you verified with a range of aligned and unaligned stack sizes
    >     around that threshhold that everything works okay?
    > 
    >     Some typos in the comment block:
    > 
    >     alligned -> aligned
    >     boundries -> boundaries
    >     I round -> We round
    > 
    >         @Thomas: os::vm_page_size() is not used in
    >         current_stack_region(), I think because of initialization
    >         dependencies.
    > 
    > 
    >     I don't see the dependency. It requires that it only be used after
    >     os::init() has been called. AFAICS the first time we will use this
    >     logic is when we attach the main thread, which happens after
    >     os::init(). 
    > 
    >     Thanks,
    >     David
    > 
    >         Best regards,
    >         Gunter
    > 
    >         From: Thomas Stüfe <thomas.stuefe at gmail.com
    >         <mailto:thomas.stuefe at gmail.com>>
    >         Date: Thursday, 17. August 2017 at 15:06
    >         To: "Haug, Gunter" <gunter.haug at sap.com
    >         <mailto:gunter.haug at sap.com>>
    >         Cc: "hotspot-dev at openjdk.java.net
    >         <mailto:hotspot-dev at openjdk.java.net>"
    >         <hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>>
    >         Subject: Re: RFR(S): 8186286: [BSD] Primary thread's stack size
    >         is reported incorrectly
    > 
    >         Hi Gunter,
    > 
    > 
    >         On Thu, Aug 17, 2017 at 2:24 PM, Haug, Gunter
    >         <gunter.haug at sap.com
    >         <mailto:gunter.haug at sap.com><mailto:gunter.haug at sap.com
    >         <mailto:gunter.haug at sap.com>>> wrote:
    >         Hi,
    > 
    >         can I please have reviews and a sponsor fort the following small
    >         bug fix:
    >         http://cr.openjdk.java.net/~ghaug/webrevs/8186286/
    >         <http://cr.openjdk.java.net/~ghaug/webrevs/8186286/>
    >         https://bugs.openjdk.java.net/browse/JDK-8186286
    >         <https://bugs.openjdk.java.net/browse/JDK-8186286>
    > 
    >         At least on Mac OS 10.12 we have observed stack sizes of the
    >         primary thread not aligned to pages boundries. This can be
    >         provoked by e.g. setrlimit() (ulimit -s xxxx in the shell).This
    >         voids the computation of the addresses of the guard pages.
    > 
    >         Fix:
    >         Apparently Mac OS actually rounds upwards to next multiple of
    >         page size however, it is conservative to round downwards here to
    >         be on the safe side.
    > 
    >         Thanks and best regards,
    >         Gunter
    > 
    >         Thanks for the patch!
    > 
    >         Very minor nits: what Alexey wrote (we also have "is_aligned").
    >         Plus, any reason not to use os::vm_page_size()? Initialization
    >         dependencies?
    > 
    >         Kind Regards, Thomas
    > 
    > 
    > 
    >