openjdk9 on Freebsd

[David Holmes]

FYI I filed:

https://bugs.openjdk.java.net/browse/JDK-8154715

David
-----

On 21/03/2016 10:45 AM, David Holmes wrote:
> A further followup/clarification ...
>
> On 21/03/2016 8:20 AM, David Holmes wrote:
>> Sorry I didn't seem this before my previous reply ...
>>
>> On 21/03/2016 4:53 AM, Brian Gardner wrote:
>>> I looked into the origination of other calls to set_thread(NULL) in
>>> openjdk8.
>>>
>>>   *
>>> hotspot/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepThread.cpp:163:
>>>
>>>
>>>     ThreadLocalStorage::set_thread(NULL);
>>>   *
>>> hotspot/src/share/vm/gc_implementation/shared/concurrentGCThread.cpp:91:
>>>     ThreadLocalStorage::set_thread(NULL);
>>>   * hotspot/src/share/vm/runtime/thread.cpp:1351:
>>>     ThreadLocalStorage::set_thread(NULL);
>>>
>>> These are use cases where set_thread(NULL) was called roughly towards
>>> the end of the threads run() method.  Annotating them showed they’ve
>>> been around prior to being open sourced.  At some point in time there
>>> was an effort made to ensure set_thread(NULL) was always called on all
>>> threads, and there is a degradation in openjdk9 in these regards.
>>
>> Any thread that terminates explicitly will do this as part of the Thread
>> destructor. For other threads it didn't seem necessary given they would
>> terminate only when the process terminated.
>
> Some non-JavaThreads can actually terminate but AFAICS fail to ever
> destroy the associated Thread instance. That would seem to be a bug in
> itself. If we did ensure the destructor was called on these threads then
> that was also deal with the removal of the set_thread(NULL). If the
> threads do not terminate however, and are still running when the VM
> terminates, then there is no place to put the "missing"
> set_thread(NULL). Note this applies to Java threads to - any Java thread
> still running at VM termination never has set_thread(NULL) called.
>
> So I'm still somewhat at a loss to understand when a "missing"
> set_thread(NULL) can cause a problem at VM termination?
>
> Thanks,
> David
>
>>> If cleaning things up in the Thread destructor (Thread::~Thread) worked
>>> on all platforms, it would be a clean way to handle the cleanup.  But
>>> since this doesn’t get called on all platforms, this logic should have
>>> been moved to JavaThread::run().  WatcherThread::run initializes it’s
>>> TLS and cleans it up, why not have JavaThread::run() do the same?  I
>>> don’t like JavaThread::run leaving it up to all it’s implementations to
>>> clean themselves up.
>>
>> This is not a platform specific issue. Thread::~Thread gets called, or
>> not, for the same set of threads on all platforms. The VMThread, nor
>> those other threads, is not a JavaThread so JavaThread::run does not
>> come into it. And for JavaThreads we don't want to do this cleanup in
>> JavaThread::run because we may still need to refer to the current
>> thread, so it is done during Thread::~Thread when the thread has
>> "terminated" as far as the VM is concerned.
>>
>> The problem at hand only affects threads that never terminate/detach.
>> They previously would call set_thread(NULL) (though why is unclear), but
>> after my changes they don't. But that can be rectified and as I said I
>> will file a bug to handle that.
>>
>> Please note however I am heading out on vacation in two days so this is
>> unlikely to get fixed until I return in a couple of weeks.
>>
>> Thanks,
>> David
>>
>>> Brian Gardner
>>>
>>>
>>>> On Mar 19, 2016, at 11:46 PM, Brian Gardner <openjdk at getsnappy.com
>>>> <mailto:openjdk at getsnappy.com>> wrote:
>>>>
>>>> Yes, I was referring to the call to
>>>> ThreadLocalStorage::set_thread(NULL).
>>>>
>>>> The problem I'm trying to resolve are the pthread destructors being
>>>> called repeadly.  On linux this isn’t a problem because the destructor
>>>> is called 4 times then silently gives up.  On FreeBSD the destructor
>>>> is called 4 times then prints a warning to stderr, which is a problem,
>>>> although it is harmless.
>>>>
>>>> The message below from the original thread states there are three
>>>> scenarios threads fall into in regards to the initial commit. The
>>>> third scenario is the problem scenario I just mentioned and while it
>>>> is ok on Linux, it isn’t ok on Freebsd because of the warnings to
>>>> stderr.
>>>>
>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2014-February/010796.html
>>>>
>>>>
>>>> I didn’t articulate it very well but I was trying to say that calling
>>>> ThreadLocalStorage::set_thread(NULL) durning thread cleanup is the
>>>> proper way to prevent the destructor from being called repeatedly.  I
>>>> actually can’t think of an alternate way to do this.
>>>> In openjdk8 all threads clean themselves by calling
>>>> ThreadLocalStorage::set_thread(NULL). I’m going to do some more
>>>> research to locate the change sets for these calls. Perhaps they are
>>>> isolated to bsd-port branch. I’ll let everyone know what I find.
>>>>
>>>> Kind regards,
>>>> Brian Gardner
>>>>
>>>>
>>>>> On Mar 18, 2016, at 2:57 PM, David Holmes <david.holmes at oracle.com
>>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>>
>>>>> On 19/03/2016 2:58 AM, Brian Gardner wrote:
>>>>>> That explains the destructor.  Looking at the initial change set that
>>>>>> came out of this bug, we also see the first spot where we set the TLS
>>>>>> current thread is set to NULL
>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/469835cd5494
>>>>>
>>>>> Are you referring to:
>>>>>
>>>>> +  // Thread destructor usually does this.
>>>>> +  ThreadLocalStorage::set_thread(NULL);
>>>>>
>>>>> ? That's only there because the VMThread destructor is never called.
>>>>>
>>>>>> So I think it’s safe to say that setting TLS current thread to
>>>>>> NULL is
>>>>>> the correct way to set the state to "destroying thread" and
>>>>>> preventing
>>>>>> the destructor from looping indefinitely.
>>>>>
>>>>> I'm not sure exactly what you mean.
>>>>>
>>>>>>
>>>>>> Here is the relevant comment from Andreas:
>>>>>>
>>>>>> --------------------------------
>>>>>>
>>>>>> I did find a way to change the JVM to workaround this problem:
>>>>>> By creating a destructor for the thread pointer TLS we can restore
>>>>>> the
>>>>>> value after pthread has set it to NULL.
>>>>>> Then when the native code destructor is run the thread pointer is
>>>>>> still
>>>>>> intact.
>>>>>>
>>>>>> Restoring a value in a pthread TLS is explicitly supported
>>>>>> according to
>>>>>> the man page for pthread_key_create, and it will call the
>>>>>> destructor for
>>>>>> the restored value again.
>>>>>> One would have to keep some extra state to make sure the
>>>>>> destructor is
>>>>>> only called twice, since a pthread implementation is allowed to
>>>>>> call the
>>>>>> destructor infinite times as long as the value is restored.
>>>>>>
>>>>>> On my system pthread calls the destructor a maximum of four times, so
>>>>>> the attached JVM patch was sufficient as a proof of concept.
>>>>>>
>>>>>> ————————————————
>>>>>
>>>>> We implemented the basic patch, we don't do anything to ensure it was
>>>>> called at most twice. We expect all well behaving apps to detach
>>>>> threads from the JVM before they terminate.
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>>> On Mar 17, 2016, at 9:02 PM, David Holmes <david.holmes at oracle.com
>>>>>>> <mailto:david.holmes at oracle.com>
>>>>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>>>>
>>>>>>> Thomas writes:
>>>>>>>> Hi Brian,
>>>>>>>>
>>>>>>>>
>>>>>>>>> The next patches where less straightforward.  When running java
>>>>>>>>> I was
>>>>>>>>> getting a ton of messages like:
>>>>>>>>> Thread 832744400 has exited with leftover thread-specific data
>>>>>>>>> after 4
>>>>>>>>> destructor iterations
>>>>>>>>> After doing a lot of digging and debugging on Linux, I found the
>>>>>>>>> code path
>>>>>>>>> for Linux was identical for Freebsd and the cleanup destructor
>>>>>>>>> was being
>>>>>>>>> executed 4 times just like Freebsd, the difference being that
>>>>>>>>> Freebsd would
>>>>>>>>> print out this benign warning while Linux would just ignore it.
>>>>>>>>> The
>>>>>>>>> problem is that all threads that are created and initialize TLS
>>>>>>>>> current
>>>>>>>>> thread data, must clean them up by explicitly setting the TLS
>>>>>>>>> current
>>>>>>>>> thread to null.  I’ve come up with two approaches to accomplish
>>>>>>>>> this.
>>>>>>>>>
>>>>>>>>> clean up TLS current thread at end of ::run functions similar to
>>>>>>>>> how
>>>>>>>>> it's
>>>>>>>>> done in openjdk8.
>>>>>>>>>
>>>>>>>>> http://brian.timestudybuddy.com/webrev/hotspot__clear_thread_current/webrev/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> clear current thread before exiting java_start to avoid warnings
>>>>>>>>> from
>>>>>>>>> leftover pthread_setspecific data
>>>>>>>>>
>>>>>>>>> http://brian.timestudybuddy.com/webrev/hotspot__clear_thread_current_alt/webrev/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>> I do not think this is a real leak. From what I remember of how
>>>>>>>> the glibc
>>>>>>>> implements TLS, setting the TLS slot value to NULL would not in
>>>>>>>> itself
>>>>>>>> delete anything. In VM, this slot keeps the pointer to the current
>>>>>>>> Thread*,
>>>>>>>> which is correctly deleted at the end of the thread (void
>>>>>>>> JavaThread::thread_main_inner()).
>>>>>>>>
>>>>>>>> Digging further, I found the pthread key destructor
>>>>>>>> "restore_thread_pointer(void* p)" in threadLocalStorage_posix.cpp:
>>>>>>>>
>>>>>>>> // Restore the thread pointer if the destructor is called. This
>>>>>>>> is in
>>>>>>>> case
>>>>>>>> // someone from JNI code sets up a destructor with
>>>>>>>> pthread_key_create
>>>>>>>> to run
>>>>>>>> // detachCurrentThread on thread death. Unless we restore the
>>>>>>>> thread
>>>>>>>> pointer we
>>>>>>>> // will hang or crash. When detachCurrentThread is called the key
>>>>>>>> will be
>>>>>>>> set
>>>>>>>> // to null and we will not be called again. If
>>>>>>>> detachCurrentThread is
>>>>>>>> never
>>>>>>>> // called we could loop forever depending on the pthread
>>>>>>>> implementation.
>>>>>>>> extern "C" void restore_thread_pointer(void* p) {
>>>>>>>> ThreadLocalStorage::set_thread((Thread*) p);
>>>>>>>> }
>>>>>>>>
>>>>>>>> So, it seems we even reset deliberately the thread pointer to a
>>>>>>>> non-NULL
>>>>>>>> value. The comment claims that we reset the Thread* value in case
>>>>>>>> there is
>>>>>>>> another user-provided destructor which runs afterwards and which
>>>>>>>> does detachCurrentThread () which would require
>>>>>>>> Thread::current() to
>>>>>>>> work.
>>>>>>>> But there a details I do not understand:
>>>>>>>>
>>>>>>>> - At this point, should the Thread* object not already be
>>>>>>>> deallocated, so
>>>>>>>> this would be a dangling pointer anyway?
>>>>>>>>
>>>>>>>> - Also, according to Posix, this is unspecified. Doc on
>>>>>>>> pthread_setspecific() states: "Calling pthread_setspecific() from a
>>>>>>>> thread-specific data destructor routine may result either in lost
>>>>>>>> storage
>>>>>>>> (after at least PTHREAD_DESTRUCTOR_ITERATIONS attempts at
>>>>>>>> destruction) or
>>>>>>>> in an infinite loop."
>>>>>>>>
>>>>>>>> - In jdk8, we did reset the slot value to NULL before Thread exit.
>>>>>>>> So, in
>>>>>>>> this case detachCurrentThread() from a pthread_key destructor
>>>>>>>> should not
>>>>>>>> have worked at all.
>>>>>>>>
>>>>>>>> Could someone from Oracle maybe shed light on this?
>>>>>>>
>>>>>>> Please see the following discussion and bug report:
>>>>>>>
>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2014-February/010759.html
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Note I don't follow this list so please include me directly in any
>>>>>>> follow-ups if needed.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>
>>>>
>>>