[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR[9u-dev] 8130425: libjvm crash due to stack overflow in executables with 32k tbss/tdata
From: Kevin Walls <kevin.walls () oracle ! com>
Date: 2016-02-26 10:08:20
Message-ID: 56D02414.9090808 () oracle ! com
[Download RAW message or body]
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@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@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.
> >
> >
> >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic