RFR(M): 8169373: Work around linux NPTL stack guard error.
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Nov 21 16:37:09 UTC 2016
On 11/21/16 3:31 AM, Lindenmaier, Goetz wrote:
> Hi Coleen, David,
>
> the cleanups we were talking about are in this webrev, which I would like to
> apply on top of 8169373:
> http://cr.openjdk.java.net/~goetz/wr16/8169373-ppc-stackFix
There are four webrevs here:
webrev.01/ 2016-Nov-10 15:33:11 - Directory
webrev.02/ 2016-Nov-16 14:34:26 - Directory
webrev.cleanup/ 2016-Nov-21 10:11:36 - Directory
webrev.minStSz/ 2016-Nov-16 14:39:09 - Directory
and I'm not sure which I should be looking at... :-)
> The code is identical in all the os_cpu files.
> (Curently it's edited on top of the other change).
>
> I would understand if you don't like the other changes I propose
> For 9. But I still think this is just how you expect this to work, the
> minimum stack size is independent of page size and thus should
> be specified without considering the guard pages. And therefore
> it should go to 9 where we already changed a lot wrt. stack sizes.
>
> Coleen, would you mind pointing me to the bugs Dan and Ron are
> working on? I'd like to understand the possible conflicts.
The fix for 8140520 was not done for Win* because we (Ron and I)
could not get reliable stack size behavior out of Win* like we
got for the *NIX platforms. I don't remember if I've filed the
follow up bugs for 8140520, but, in any case, I'm not working
on thread stack size issues at the moment.
Dan
>
> Best regards,
> Goetz.
>
>> -----Original Message-----
>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>> bounces at openjdk.java.net] On Behalf Of David Holmes
>> Sent: Sonntag, 20. November 2016 02:21
>> To: Coleen Phillimore <coleen.phillimore at oracle.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: RFR(M): 8169373: Work around linux NPTL stack guard error.
>>
>> On 19/11/2016 12:40 AM, Coleen Phillimore wrote:
>>>
>>> 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 meant the entire current_stack_region method to which this new
>> fragment has been added.
>>
>> David
>>
>>> 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());
>>>>>>> }
>>>>>>>
More information about the hotspot-runtime-dev
mailing list