RFR(s): 8212173: Thread._stack_base/_stack_size initialized too late for new threads (original) (raw)
Volker Simonis volker.simonis at gmail.com
Fri Oct 26 09:45:40 UTC 2018
- Previous message: RFR(s): 8212173: Thread._stack_base/_stack_size initialized too late for new threads
- Next message: RFR(s): 8212173: Thread._stack_base/_stack_size initialized too late for new threads
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
HI Thomas,
your change looks good. Please find some minor comments below:
os_solaris.cpp/os_windows.cpp
- why don't you set "thread" to null after the return from call_run() like on the other platforms? Or maybe the comment is enough and you don't have to set it to null at all? There's no need to do a new webrev for this.
Thanks, Volker On Thu, Oct 25, 2018 at 8:02 PM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
Ping... May I please have a second review? Thanks! On Mon, Oct 22, 2018 at 6:56 PM Thomas Stüfe <thomas.stuefe at gmail.com> wrote: > > Dear all, > > may I have please reviews for the following fix: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8212173 > cr: http://cr.openjdk.java.net/~stuefe/webrevs/8212173-thread-stack-and-base-initialized-too-late/webrev.02/webrev/ > > See also prior mail discussions: [1] > > Wwhen iterating the thread list, threads may be visible which have > stack base/size not yet set. Calling stackbase() will yield NULL, > stacksize() 0 or it will assert. The reason for this is that there > exist a race: Thread::stackbase and ::stacksize are initialized > too late, when the Thread has already been added to the threads list. > > My solution to this problem is basically to move the initialization of > Thread::stackbase and ::stacksize to an earlier point at which the > thread has not yet been added to the threads list. > > Unfortunately, this is more difficult that it sounds. Retrieving > thread stack base and size needs to be done from within the thread in > question, and only on Linux and BSD we have the opportunity to execute > code in the new child thread before its Thread object is added to the > thread list. So, my patch solves the problem completely only on Linux > and BSD. On Windows, Aix and Solaris it only minimizes the chance for > it to happen. > > For more details, please see bug description and the discussion in [1]. > > ---- > > My changes in detail: > > - Thread::recordstackbaseandsize(): I removed all code which needs > the surrounding Thread object being initialized, or which needs > Thread::current() to be set. That makes the function safe to be called > from the very start of the child thread's life (start of > threadnativeentry()), where nothing is initialized yet. > > - I moved the call to Thread::recordstackbaseandsize() out of all > ::run() functions into the very start of the > threadnativeentry() on every platform. > > - The parts I moved out of Thread::recordstackbaseandsize() are: > NMT thread stack initialization and Logging. I needed to find a new > place for them. I wanted them to run at the end of the child thread > initialization, when the child Thread is already initialized and > Thread::current is initialized too. So I moved them to just before > ::run() is called. > > - To prevent code duplication, I changed: > > public virtual Thread::run() > > to > > public (nonvirtual) Thread::callrun(); > protected virtual Thread::run() = 0; > > Thread::callrun() now is the place to put common, > platform-independent and -independent > initializations. There I put the NMT stuff and logging. After > ::run() returns, I put common cleanup checks. > > Other changes: > > - We had a function os::initializethread() which only existed for the > sole benefit of Solaris, where if gives the platform a stab at > correcting the stack base, size for a certain corner case when we run > on primordial thread. On all other platform-cpu-combinations, this > function was empty. I removed all empty implementations and renamed > the function to the much more fitting, Solaris only > "os::Solaris::correctstackboundariesforprimordialthread(Thread* > thr)". > > - When ::run() returns, the Thread object may have > been deleted by the child class run() function. To avoid errors I set > the thread pointer to zero (where possible) to prevent accessing it to > reference members. > > - Since Thread::recordstackbaseandsize() does not register the > thread stack with NMT anymore, I needed to fix those places where the > former is called for attached threads and add that NMT registration > manually. > > - I made Thread::clearthreadcurrent() a static function since it > needs no members of Thread (unlike Thread::initializethreadcurrent() > which needs the this pointer to hook it into TLS). That makes it > possible to use clearthreadcurrent() without having a valid Thread > object, which is the case if ::run() deleted the > Thread. > > ---- > > That's about it. > > The change ran through jdk-submit tests without errors. > > Thank you for reviewing. > > ..Thomas > > [1] http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-October/030516.html
- Previous message: RFR(s): 8212173: Thread._stack_base/_stack_size initialized too late for new threads
- Next message: RFR(s): 8212173: Thread._stack_base/_stack_size initialized too late for new threads
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]