RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata

[Kevin Walls]

Thanks Martin - I think we're going to proceed with the arguably 
somewhat ugly change as it does solve a real problem, although somewhat 
unusual to have such large thread local sizes for it to matter.  And we 
may have to deal with a thread library that takes away stack space for 
thread local data for a while.

Thanks
Kevin


On 25/02/2016 16:30, Martin Buchholz wrote:
> My hope is that we eventually eliminate StackOverflowError by
> implementing dynamically growable thread stacks.
>
> Until then, my hope is that hotspot give the thread the memory it asks
> for, for actually storing stack frames, and thread locals should not
> steal space from that.
>
> The system property introduced in this change is an ugly workaround for that.
>
> On Thu, Feb 18, 2016 at 2:24 AM, Kevin Walls <kevin.walls at oracle.com> wrote:
>> Hi Cheleswer,
>>
>> Looks good to me.
>>
>> Thanks
>> Kevin
>> (Also, as one of the comments was that there may be no real cost to using
>> default stack sizes here (on most systems...?), having
>> jdk.lang.processReaperUseDefaultStackSize gives us a way to test that
>> widely, and one day the 32k may be able to disappear.)
>>
>>
>>
>>
>>
>> On 17/02/2016 15:17, cheleswer sahu wrote:
>>
>> Hi,
>> I have made changes in the property name
>> (jdk.lang.processReaperUseDefaultStackSize) and code as suggested in the
>> earlier reviews.
>>
>>   --- old/src/java.base/share/classes/java/lang/ProcessHandleImpl.java
>> 2016-02-17 18:48:10.545639999 +0530
>> +++ new/src/java.base/share/classes/java/lang/ProcessHandleImpl.java
>> 2016-02-17 18:48:10.081639999 +0530
>> @@ -81,9 +81,8 @@
>>                   ThreadGroup systemThreadGroup = tg;
>>
>>                   ThreadFactory threadFactory = grimReaper -> {
>> -                    // Our thread stack requirement is quite modest.
>> -                    Thread t = new Thread(systemThreadGroup, grimReaper,
>> -                            "process reaper", 32768);
>> +                    long stackSize =
>> Boolean.getBoolean("jdk.lang.processReaperUseDefaultStackSize") ? 0 : 32768;
>> +                    Thread t = new Thread(systemThreadGroup, grimReaper,
>> "process reaper", stackSize);
>>                       t.setDaemon(true);
>>                       // A small attempt (probably futile) to avoid priority
>> inversion
>>                       t.setPriority(Thread.MAX_PRIORITY);
>>
>> I would really like to thanks "Martin" for the patch
>> (http://cr.openjdk.java.net/~martin/webrevs/openjdk9/tls-size-guarantee/)
>> which he has provided. Since we support a wider range of glibc versions and
>> platform using patch will
>> require more testing and work. I think the use of system property for this
>> issue will be safer at this point of time.
>>
>> Regards,
>> Cheleswer
>>
>>
>> On 1/19/2016 5:40 PM, David Holmes wrote:
>>
>> On 19/01/2016 9:53 PM, Kevin Walls wrote:
>>
>> |
>> Hi Cheleswer, I think Martin is suggesting something like:
>> |
>>
>> // Use a modest stack size, unless requested otherwise:
>> long stackSize = Boolean.getBoolean("processReaperUseDefaultStackSize") ? 0
>> : 32768;
>>
>>
>> Someone from core-libs needs to chime on what the appropriate namespace for
>> such a property would be.
>>
>> David
>> -----
>>
>> Thread t = new Thread(systemThreadGroup, grimReaper, "process reaper",
>> stackSize);
>>
>> |||
>> If that tests OK for you, it may be the way we can go ahead with the
>> point fix for this.
>>
>> Regards
>> Kevin
>> |
>> On 18/01/2016 16:52, Martin Buchholz wrote:
>>
>> Historical note - I never liked having a reaper thread for each
>> subprocess, but no other reliable implementation is known.  Given the
>> potential for many reaper threads, I at least wanted to keep the
>> memory waste low.
>>
>> On Wed, Jan 13, 2016 at 2:25 AM, cheleswer sahu
>> <cheleswer.sahu at oracle.com>  wrote:
>>
>> +                   Thread t = null;
>> +                   if
>> (Boolean.getBoolean("processReaperUseDefaultStackSize")) {
>> +                       t = new Thread(systemThreadGroup, grimReaper,
>> "process reaper");
>> +                    } else {
>> +                       // Our thread stack requirement is quite modest.
>> +                       t = new Thread(systemThreadGroup, grimReaper,
>> "process reaper", 32768);
>> +                    }
>>
>> If we do end up using this strategy, cleaner would be to use
>> https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#Thread-java.lang.ThreadGroup-java.lang.Runnable-java.lang.String-long-
>> stackSize - the desired stack size for the new thread, or zero to
>> indicate that this parameter is to be ignored.
>>
>>
>>
>>