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

[David Holmes]

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
> 
> 
> 
>