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

[David Holmes]

On 16/02/2017 6:55 PM, Thomas Schatzl wrote:
> Hi,
>
> On Thu, 2017-02-16 at 18:09 +1000, David Holmes wrote:
>> 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;
>
> Maybe I'm wrong, but just looking at this suggestion without more
> thought: what if the value of size changes just between the above
> load_acquire and the re-read of the size, and the actual value in the
> array has not been updated yet?

Yeah I screwed up peeling out the initial read of size. That line should 
come at the end of the loop after c=c->next;

Thanks,
David

> If that optimization is done, please just peel out the first iteration.
>
> Thanks,
>   Thomas
>