Hi David,
I looked through the GC part of this webrev and I think the change
is fine.
However, it seems a bit error prone. If we decide to change the
code to, for example, terminate the AbstractGangWorker threads,
then we have to remember to insert a
ThreadLocalStorage::set_thread(NULL) call.
Could we instead add a call to
ThreadLocalStorage::set_thread(NULL), or maybe even
Thread::clear_thread_current(), in java_start?
static void *java_start(Thread *thread) {
[...]
thread->initialize_thread_current();
[...]
// call one more level start routine
thread->run();
////////// Could we call Thread::clear_thread_current(); here?
log_info(os, thread)("Thread finished (tid: " UINTX_FORMAT ",
pthread id: " UINTX_FORMAT ").",
os::current_thread_id(), (uintx) pthread_self());
return 0;
}
And get rid of the explicit calls to
ThreadLocalStorage::set_thread(NULL) you added?
Thanks,
StefanK
On 04/05/16 01:39, David Holmes wrote:
This needs attention from GC and runtime folk please.
bug: https://bugs.openjdk.java.net/browse/JDK-8154715
webrev: http://cr.openjdk.java.net/~dholmes/8154715/webrev/
tl;dr: ensure ThreadLocalStorage::set_thread(NULL) is always
called before a thread terminates.
Background:
Most system-related threads do not expect to explicitly terminate,
except sometimes as part of VM termination. Such threads don't
have their destructors called, but should.
This omission came to light due to the ThreadLocalStorage changes
in JDK-8132510. As part of that change we deleted the following
from the termination path of the VMThread:
// Thread destructor usually does this.
ThreadLocalStorage::set_thread(NULL);
The clearing of TLS seemed irrelevant to the VMThread as it
primarily is used to aid in JNI attach/detach. However Brian
Gardner reported:
http://mail.openjdk.java.net/pipermail/bsd-port-dev/2016-February/002788.html
a problem on FreeBSD caused by this change and the interaction
with the POSIX pthread TLS destructor use introduced by
JDK-8033696. Because the VMThread terminated without clearing TLS,
when the TLS-destructor was called it got into a loop which ran
four times (as happens on Linux) and then prints a warning to the
console (which doesn't happen on Linux).
This indicates we need to restore the:
ThreadLocalStorage::set_thread(NULL);
but on further consideration it seems to me that this is not
confined to the VMThread, and the most appropriate fix would be to
always invoke the Thread destructor as a thread terminates.
Solution:
Further investigation shows that calling the Thread destructor in
the thread as it terminates is not possible:
- VMThread
This is actually destroyed by the thread that terminates the VM,
but that can happen after it terminates and so we still hit the
TLS problem. The VMThread may be able to destroy itself today but
in the past this was not possible (see existing code comment), and
in the future it may also not be possible - the problem is that
the Thread destructor can interact with other VM subsystems that
are concurrently being torn down by the thread that is terminating
the VM. In the past this was the CodeHeap. So rather than
introduce something that is fragile we stick with the current
scheme but restore the ThreadLocalStorage::set_thread(NULL); -
note we can't access "this" at that time because it may already
have been de-allocated.
- WatcherThread
The WatcherThread is never destroyed today but has the same
problem as the VMThread. We can call the destructor from the VM
termination thread (and have implemented that), but not from the
WatcherThread itself. So again we just have to restore the
ThreadLocalStorage::set_thread(NULL); to fix the potential TLS
problem.
- GC Threads
There are two cases:
a) GC threads that never terminate
For these we don't need to do anything: we can't delete the thread
as it never terminates and we don't hit the TLS problem because it
never terminates. So all we will do here is add some logic to
check (in NON_PRODUCT) that we do in fact never terminate.
b) GC threads that can terminate
Despite the fact the threads can terminate, references to those
threads are stored elsewhere (WorkGangs and other places) and are
not cleared as part of the termination process. Those references
can be touched after the thread has terminated so we can not call
the destructor at all. So again all we can do (without some major
thread management reworking) is ensure that
ThreadLocalStorage::set_thread(NULL); is called before the thread
actually terminates
Testing: JPRT
RBT - runtime nightly tests
Thanks,
David