RFR(M): 8169373: Work around linux NPTL stack guard error.

[Coleen Phillimore]

On 11/18/16 1:34 AM, David Holmes wrote:
> Hi Goetz,
>
> On 17/11/2016 1:52 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> I addressed the additional topics, but in an extra change.
>>
>> First, I rounded up the NPTL bug workaround:
>> http://cr.openjdk.java.net/~goetz/wr16/8169373-ppc-stackFix/webrev.02
>>
>> I fixed os::Posix::describe_pthread_attr() and current_stack_region()
>> In the os_cpu files.
>> Interestingly, in zero,  current_stack_region() is already fixed.
>
> So on Linux, pthread_attr_getstack is also broken - in that it returns 
> the address at the end of the guard section, instead of the usable stack?
>
> Can you add error checking on the pthread_attr_getguardsize call please.
>
> The duplication of the current_stack_region static method in the 
> os-cpu files is horrible. This should be placed as a private method in 
> the os class and moved to os_linux.cpp. RFE for 10.
By duplication do you mean this bit of code?

http://cr.openjdk.java.net/~goetz/wr16/8169373-ppc-stackFix/webrev.02/src/os_cpu/linux_ppc/vm/os_linux_ppc.cpp.udiff.html

+ // Work around NPTL stack guard error.
+ size_t guard_size = 0;
+ pthread_attr_getguardsize(&attr, &guard_size);
+ *bottom += guard_size;
+ *size -= guard_size;
+

This should be cleaned up with this patch and not an RFE for 10.

I don't know the NPTL issues well enough to be a reviewer for this, but 
I don't like something I don't know about distributed in many places.

>
>
>> Then I edited an extra webrev to fix
>>   - unnecessary OS guard page on Compiler threads
>>   - unpredictable minimal stack sizes because of varying (guard) page 
>> sizes:
>> http://cr.openjdk.java.net/~goetz/wr16/8169373-ppc-stackFix/webrev.minStSz/ 
>>
>>
>> Compiler threads now don't get OS guard pages.  Saves one page per 
>> compiler thread.
>>
>> Also, the constants in the os_<os>_<cpu>.cpp files now list minimum 
>> space
>> required for frames.  I basically reduced them by the size of the 
>> guard zones.
>> Where minimum stack sizes are computed, the page size is known and the
>> minimal value is extended by the required space for the guard pages. 
>> As compiler threads
>> don't bang, no space for the shadow zone is needed.
>>
>> Should I add this additional change to 8169373?  As this only deals 
>> with minimal
>> stack sizes, and reduces these sizes, I don't see a relevant risk.
>> Only on systems with large pages the minimal sizes now might be 
>> bigger, but
>> then the old sizes would have led to immediate stack overflows.
>
> Sorry this is getting a bit too divergent and disruptive to me for 
> this stage of 9. You are redefining what the "minimum stack" values 
> mean as well as changing their actual value. (BTW I'm sure there is a 
> test of these values that would need updating.)
>
> I can't tell if it is right/wrong, necessary/unnecessary - but given 
> the recent work done in this area I'm somewhat cautious about changing 
> it again - in 9.

I think the additional changes should also wait for 10.  I like the 
change to not have shadow pages for the compiler threads (not including 
AOT compiler threads, right?), but they also collide with work that I 
think Dan is going to do for windows.   We don't want the os::Posix 
functions in these files.

http://cr.openjdk.java.net/~goetz/wr16/8169373-ppc-stackFix/webrev.minStSz/src/os_cpu/linux_x86/vm/os_linux_x86.cpp.udiff.html

Also, the work that Dan and Ron did in this area didn't change the sizes 
and I'd want Dan to have time to comment on this.

Thanks,
Coleen

>
> David
>
>>
>> Best regards,
>>   Goetz.
>>
>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Dienstag, 15. November 2016 00:54
>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>>> dev at openjdk.java.net
>>> Subject: Re: RFR(M): 8169373: Work around linux NPTL stack guard error.
>>>
>>> Hi Goetz,
>>>
>>> On 15/11/2016 3:12 AM, Lindenmaier, Goetz wrote:
>>>> Hi David,
>>>>
>>>>> Did you really intend to increase the real minimum stack from 128K to
>>>>> 256K ? (on a 64K page system)
>>>> I finally figured why I have to set such high minimum values.
>>>>
>>>> A CompilerThread is a JavaThread. It's getting yellow, red etc
>>>> pages at the top. As these are 64K on 64K systems, the compiler
>>>> thread stack has to be this big. Thus you actually need similar
>>>> values once you run a linuxx86_64 VM on a system with 64K pages.
>>>> Does this exist?
>>>
>>> No idea, sorry.
>>>
>>>> But, this means that there is no use of the OS guard pages
>>>> configured in default_guard_size().  This is called with 
>>>> 'compiler_thread'
>>>> for CompilerThreads.
>>>>
>>>> We should probably also return '0' in that case.
>>>>
>>>> What do you think?
>>>
>>> I think I agree. As you note a CompilerThread is-a JavaThread even
>>> though the ThreadType enum treats them as completely distinct:
>>>
>>>    enum ThreadType {
>>>      vm_thread,
>>>      cgc_thread,        // Concurrent GC thread
>>>      pgc_thread,        // Parallel GC thread
>>>      java_thread,
>>>      compiler_thread,
>>>      watcher_thread,
>>>      os_thread
>>>    };
>>>
>>> so anything with VM supplied guard pages doesn't need the glibc ones as
>>> well.
>>>
>>> I'm struggling to understand why os::Linux::default_guard_size is
>>> defined per CPU type? I would not expect this to need to vary based on
>>> CPU at all. ??
>>>
>>> Thanks,
>>> David
>>>
>>>> Best regards,
>>>>   Goetz.
>>>>
>>>> size_t os::Linux::default_guard_size(os::ThreadType thr_type) {
>>>>   // Creating guard page is very expensive. Java thread has HotSpot
>>>>   // guard page, only enable glibc guard page for non-Java threads.
>>>>   return (thr_type == java_thread ? 0 : page_size());
>>>> }
>>>>