[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8257831: Suspend with handshakes [v2]
From: David Holmes <david.holmes () oracle ! com>
Date: 2021-03-31 23:28:09
Message-ID: c28d877c-a027-d471-2f7e-ed181e0ce048 () oracle ! com
[Download RAW message or body]
On 1/04/2021 12:35 am, Robbin Ehn wrote:
> On Wed, 31 Mar 2021 10:48:11 GMT, Robbin Ehn <rehn@openjdk.org> wrote:
>
> > > Thanks @dcubed-ojdk - no it won't. The problem is that the signal can hit very \
> > > late in a thread's termination process, after the JavaThread destructor has \
> > > executed, so no query of JavaThread state is possible. So we needed something \
> > > in the Thread state and the SR_lock was a good enough proxy for that. It may be \
> > > possible to use a different Thread field (perhaps _ParkEvent but encapsulated \
> > > in a Thread::has_terminated() helper method).
> >
> > SR_handler is used for OS-level suspend/resume (not to conflict with this \
> > change-set). This feature is only used by JFR (AFAIK), and JFR only samples \
> > threads on it's ThreadsList. This means the JavaThread can never be terminated, \
> > hence this code will always pass.
> > If someone else is using OS-level suspend/resume without a ThreadsList, the bug \
> > is there is no ThreadsList AFAICT.
> > Since ThreadLocalStorage::thread() is cleared last in ~Thread with \
> > Thread::clear_thread_current(); may be in the ~Thread destructor. So the argument \
> > is that would be safe to read stuff from Thread but not JavaThread? Since the \
> > compiler (and CPU) may reorder and optimize away code, so I argue reading from a \
> > half destroyed object is not a great idea. E.g. Monitor* _SR_lock; is not a \
> > volatile pointer; since reading from this memory is UB after destruction, \
> > compiler is free to remove or not publish the store to NULL.
> > So I suggest either to remove this check, since the only user is using a \
> > ThreadsList and any other should also be using that too. Or call \
> > Thread::clear_thread_current() before the JavaThread destructor is called, that \
> > way we can be certain that there is no UB.
>
> I got some offline input from David, there seem to be an issue with signal being \
> delivered after the ThreadsListHandle destructor. If that is the case a ThreadsList \
> doesn't help here.
> So I suggested we keep this out of this change-set and just take another suitable \
> field to mirror what we have today.
> `ParkEvent * _ParkEvent;` ?
Yes nicely packaged as:
bool Thread::has_terminated() {
return _ParkEvent == NULL;
}
Also note:
// It's possible we can encounter a null _ParkEvent, etc., in
stillborn threads.
// We NULL out the fields for good hygiene.
ParkEvent::Release(_ParkEvent); _ParkEvent = NULL;
the comment is wrong as it can't be NULL even if stillborn. And now we
NULL it out for good hygiene and as a late-stage termination indicator.
And yes we can make _ParkEvent volatile to dissuade the compiler from
eliding the NULLing out.
Thanks,
David
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3191
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic