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

[Thomas Schatzl]

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?

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

Thanks,
  Thomas