RFR: 8168914: Crash in ClassLoaderData/JNIHandleBlock::oops_do during concurrent marking

[David Holmes]

Bah! didn't finish my edit ...

On 16/02/2017 5:54 PM, David Holmes wrote:
> On 16/02/2017 5:46 PM, Kim Barrett wrote:
>>> On Feb 16, 2017, at 2:41 AM, David Holmes <david.holmes at oracle.com>
>>> wrote:
>>>
>>> On 16/02/2017 3:53 PM, Kim Barrett wrote:
>>>>>> /src/share/vm/classfile/classLoaderData.cpp
>>>>>> 138     const juint size = OrderAccess::load_acquire(&c->size);
>>>>>>
>>>>>> I think all but the first chunk have a constant size of the
>>>>>> maximum size, so no need for acquire except for the first.
>>>>>
>>>>> The size is incremented on each add, so this acquire sync's with
>>>>> the release of the new size, so that we are sure to see the oop
>>>>> that was just added.
>>>>>
>>>>> Though given the races between add and oops_do it may not actually
>>>>> matter.
>>>>
>>>> By “first” I mean the first chunk.  only the first chunk’s size is
>>>> subject to modification.  The release_store of _head that made the
>>>> first chunk accessible will be after the setting of the next chunk’s
>>>> size to its final (max) value.  The corresponding load_acquire of
>>>> _head ensures any _next chunks are visibly complete.  It’s the same
>>>> rationale for _next not requiring any special handling. At least,
>>>> that’s how it looks to me right now; am I missing something.
>>>
>>> The _head chunk has its _size incremented on each add().
>>>
>>> David
>>
>> Agreed, and a load_acquire is needed for the _head chunk.
>>
>> I’m suggesting an ordinary load is sufficient for any _next chunks as
>> we loop down the list.
>
> Ah! Now I get what you mean - sorry. Yes only the first loop iteration
> needs the load-acquire. So just:
>
> Chunk* c = (Chunk*) OrderAccess::load_ptr_acquire((volatile
> intptr_t*)&_head);
> const juint size = (c == NULL ? 0 : OrderAccess::load_acquire(&c->size));
> while (c != NULL) {
>     size = OrderAccess::load_acquire(&c->size);

       size = c->size;

David
-----
>     ...
> }
>
>
> Cheers,
> David
>>