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

[Martin Buchholz]

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