RFR (XXS): 7006810: G1: Introduce peace-of-mind checking in the Suspendible Thread Set

[Srinivas Ramakrishna]

On Thu, Apr 9, 2015 at 2:00 AM, Per Liden <per.liden at oracle.com> wrote:

> Hi Joe/Ramki,
>
> On 2015-04-09 09:58, Srinivas Ramakrishna wrote:
>
>> Hi Joe --
>>
>> On Wed, Apr 8, 2015 at 4:27 PM, Joseph Provino
>> <joseph.provino at oracle.com <mailto:joseph.provino at oracle.com>> wrote:
>>
>> ...
>>     On 4/6/2015 2:20 PM, Srinivas Ramakrishna wrote:
>>
>>> ...
>>
>>     Do we allow threads to recursively enter the suspendible thread
>>>     set in nested fashion? I'm guessing we don't, in which case, you
>>> would
>>>     use this information local to the thread to prevent/forbid such
>>>     recursive entries or exits when it isn't part of the set, by
>>> asserting
>>>     that the bit isn't set when a thread joins the set and never clear
>>>     before a thread leaves the set. I understand that there's a scoped
>>>     join/leave abstraction via SuspendibleThreadSetJoiner, but there
>>>     also seem to be a bunch of naked join/leave calls, so the
>>>     extra checking would likely be worthwhile.
>>>
>>     This is a good question.  If I put in checks in join / leave
>>
>>     void SuspendibleThreadSet::join() {
>>        assert(Thread::current()->is_suspendible_thread() == false,
>>          "Thread already joined");
>>
>>     void SuspendibleThreadSet::leave() {
>>        //assert(Thread::current()->is_suspendible_thread(),
>>        //  "Thread not joined");
>>
>>     jprt hits the assert in join.
>>
>>     It doesn't seem right to call join if a thread is already joined.
>>     Would it be correct to just return if trying to rejoin or trying to
>>     leave when not joined?
>>
>>
>> That would imply (along with the use of the scoped join/leave
>> abstraction) that nested joins/leaves are allowed.
>> I'd suggest then, that a thread keep track of the number of times it
>> joined and left by using a full-fledged
>> counter instead of a boolean bit.
>> Then your asserts would check that it wasn't trying to leave when its
>> count was 0.
>>
>
> Even if the STS allows nested joins (or rather, today it doesn't check for
> it), to the best of my knowledge we don't have any such threads. Joe, I
> suggest that you look at the threads using STS (not that many) to confirm
> that this is the case. I don't think we should allow it, and therefor I
> think the thread state should be kept as a bool rather than a counter. As I
> mentioned in my other reply, there's a bug here. Since _suspendible_thread
> is never properly initialized it might look like we have already joined,
> when in fact we have just forgot to initialize it. I suspect that's the
> reason why it might look like we have nested joins.
>
>
Ah, OK. Hopefully Joe's testing after fixing the initialization confirmed
that that was what was triggering the assert. I agree that it makes sense
to keep it simple and disallow nested joins, if possible. (Haven't looked
at the exit protocol carefully, but I will, as well as looking at the newer
webrev.)

-- ramki
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150409/5d537d1f/attachment.htm>