On 28/11/2018 5:46 pm, Thomas Stüfe wrote:
Are you sure?
No. I'm completely confusing myself - too many different "thread id's" :(
On Linux, pthread id is set in the parent thread, after pthread_create
returns. Only the kernel thread id is set by the child (I find this
duality confusing). thread_cpu_time uses pthread_id. So, on Linux, it
may or may not be set before the thread stack boundaries are
initialized, depending on whether the native entry function ran before
the parent thread continued.
Yes you are right. The code is still broken but for a different reason.
I may just fix this all in JDK-8214097 - Rework thread initialization
and teardown logic - by only adding to the list after the thread has run
its own initialization logic. Then the guard I just added to the
management code can be deleted again.
Thanks,
David
..Thomas
On Wed, Nov 28, 2018 at 8:35 AM David Holmes <david.hol...@oracle.com> wrote:
#$%$#@! I missed the fact that the thread id is set _after_ we do
record_stack_base_and-size(). I thought that was in call_run() not
native_thread_entry().
<sigh> New bug being filed.
Thanks,
David
On 28/11/2018 5:26 pm, David Holmes wrote:
Hi Thomas,
On 28/11/2018 4:30 pm, Thomas Stüfe wrote:
Hi David,
I admit I still do not really get it..
E.g. for GC threads:
We have
static ConcurrentMarkSweepThread*
ConcurrentMarkSweepThread::start(CMSCollector* collector);
which creates a ConcurrentMarkSweepThread and then calls
os::create_thread() to hook it up with an OSThread and start it.
ConcurrentMarkSweepThread derives from NonJavaThread, which in its
constructor adds the thread object to the list.
So there is a time gap where the NJT is part of the list, but
Thread::_osthread is still NULL.
In ThreadTimesClosure::do_thread(), we call
os::thread_cpu_time(thread)->fast_cpu_time(thread)->os::Linux::pthread_getcpuclockid(thread->osthread()->pthread_id(),
&clockid);
Should we then not crash when dereferencing the NULL
osthread()->pthread_id()? Why do we crash inside
pthread_getcpuclockid?
If I look further into os::create_thread(), I see that there is
another, smaller time gap where we create OSThread and anchor it into
the Thread object:
thread->set_osthread(osthread);
and then later, after pthread_create is thru, set its thread id:
// Store pthread info into the OSThread
osthread->set_pthread_id(tid);
When OSThread is created, we call OSThread::pd_initialize() and set
its _threadid to 0. We do this in the constructor, before anchoring
the OSThread in its Thread.
So for my understanding, we should have two situations:
(1)- NJT is in list, but its _osthread member is NULL. In that case I
would expect a different crash.
(2)- NJT is in list, _osthread is set, but its _thread_id is NULL.
Yes both situations may be possible. In the related bug report only the
0 thread_id was observed.
(Modulo any concurrency issues, e.g. could the
"thread->set_osthread(osthread);" be visible before OSThread is
initialized?
Depending on what is happening, a fix for (1) would probably be a
querying for osthread==NULL, a fix for (2) would be to guard os::---
functions - well, at least os::thread_cpu_time() - to disregard
Threads with pthread_t == 0. Both fixes seem better to me than
querying the stacksize() when walking the list - that seems a bit
awkward.
I find it awkward that the list is populated with partially constructed
threads in the first place. This particular code is only interested in
live threads that can safely be queried. The stack_size() check is a
surrogate for "this is a live thread that can be queried".
--
P.s.
ConcurrentGCThread::ConcurrentGCThread() :
_should_terminate(false), _has_terminated(false) {
};
I was surprised to see no invocation to the base class ctor in the
initializer list. I was not aware that this was even possible. For
code clearness, I would prefer the call to the base class ctor to be
explicit.)
I assume it is implicit. But yes it should be explicit.
Cheers,
David
--
Cheers, Thomas
On Wed, Nov 28, 2018 at 3:58 AM David Holmes <david.hol...@oracle.com>
wrote:
Sorry for the delayed response.
On 21/11/2018 3:01 pm, Kim Barrett wrote:
On Nov 20, 2018, at 3:50 AM, David Holmes <david.hol...@oracle.com>
wrote:
After discussions with Kim I've decided to split out the NJT list
update into a separate RFE:
https://bugs.openjdk.java.net/browse/JDK-8214097
So only the change in management.cpp needs reviewing and testing.
Updated webrev:
http://cr.openjdk.java.net/~dholmes/8212207/webrev.v2/
Looks good.
Thanks Kim.
I've decided to stick with this simple fix for NJTs only.
David
Thanks,
David
On 20/11/2018 10:01 am, David Holmes wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8212207
webrev: http://cr.openjdk.java.net/~dholmes/8212207/webrev/
There is an internal management API that reports CPU times for
NonJavaThreads (NJTs). That functionality requires a valid/live
target thread so that we can use its pthread_t identity to obtain
its CPU clock via pthread_getcpuclockid().
There is an iteration mechanism for NJTs in which the NJT is
registered during its constructor and de-registered during its
destructor. A thread that has only been constructed has not yet
executed and so is not a valid target for this management API.
This seems to be the cause of failures reported in this bug (and
JDK-8213434). Registering a NJT only when it starts executing is
an appealing fix for this, but that impacts all current users of
the NJT list and straight-away causes a problem with the
BarrierSet initialization logic. So I don't attempt that.
Instead the first part of the fix is for
ThreadTimesClosure::do_thread to skip threads that have not yet
executed - which we can recognize by seeing an uninitialized (i.e.
zero) stackbase.
A second part of the fix, which can be deferred to a separate RFE
for NJT lifecycle management if desired, tackles the problem of
encountering a terminated thread during iteration - which can also
lead to SEGVs. This can arise because NJT's are not actually
"destructed", even if they terminate, and so they never get
removed from the NJT list. Calling destructors is problematic
because the code using these NJTs assume they are always valid. So
the fix in this case is to move the de-registering from the NJT
list out of the destructor and into the Thread::call_run() method
so it is done before a thread actually terminates. This can be
considered a first step in cleaning up the NJT lifecycle, where
the remaining steps touch on a lot of areas and so need to be
handled separately e.g. see JDK-8087340 for shutting down WorkGang
GC worker threads.
Testing: tiers 1 -3
I should point out that I've been unable to reproduce this failure
locally, even after thousands of runs. I'm hoping Zhengyu can test
this in the conditions reported in JDK-8213434.
Thanks,
David
