RFR: 8166811: Missing memory fences between memory allocation and refinement

[Thomas Schatzl]

Hi,

On Thu, 2016-11-10 at 13:20 -0500, Kim Barrett wrote:
> > 
> > On Nov 8, 2016, at 7:52 AM, Thomas Schatzl <thomas.schatzl at oracle.c
> > om> wrote:
> > On Tue, 2016-10-25 at 19:13 -0400, Kim Barrett wrote:
> > > 
> > > There is still a situation where processing can fail, namely an
> > > in-progress humongous allocation that hasn't set the klass
> > > yet.  We continue to handle that as before.
> > - I am not completely sure about whether this case is handled
> > correctly. I am mostly concerned that the information used before
> > the fence may not be the correct ones, but the checks expect them
> > to be valid.
> > 
> > Probably I am overlooking something critical somewhere.
> > 
> > A: allocates humongous object C, sets region type, issues
> > storestore, sets top pointers, writes the object, and then sets C.y
> > = x to mark a card
> > 
> > Refinement: gets card (and assuming we have no further
> > synchronization around which is not true, e.g. the enqueuing)
> > 
> >  592   if (!r->is_old_or_humongous()) {
> > 
> > assume refinement thread has not received the "type" correctly yet,
> > so must be Free. So the card will be filtered out incorrectly?
> > 
> > That is contradictory to what I said in the other email about the
> > comment discussion, but I only thoroughly looked at the comment
> > aspect there. :)
> > 
> > I think at this point in general we can't do anything but
> > !is_young(), as we can't ignore cards in "Free" regions - they may
> > be for cards for humongous ones where the thread did not receive
> > top and/or the type yet?
> > 
> > - assuming this works due to other synchronization,
> This is the critical point.  There *is* synchronization there.

Okay, thanks. I just wanted to make sure that we are aware of that we
are using this other synchronization here.

> In the scenario described, the card that was marked and enqueued
> after the object was created will pass through some synchronization
> barriers (full locks, perhaps someday lock-free but with appropriate
> memory barriers) along the way to refinement.
> 
> This is the "easy" case.  If only it were that simple...
> 
> The additional checks are to deal with the possibility of stale
> cards.
> 
> > 
> > […] I have another
> > similar concern with later trimming:
> > 
> > 653 } else {
> > 654   // Non-humongous objects are only allocated in the old-gen
> > during
> > 655   // GC, so if region is old then top is stable.  Humongous
> > object
> > 656   // allocation sets top last; if top has not yet been set,
> > then
> > 657   // we'll end up with an empty intersection.
> > 658   scan_limit = r->top();
> > 659 }
> > 660 if (scan_limit <= start) {
> > 661   // If the trimmed region is empty, the card must be stale.
> > 662   return false;
> > 663 }
> > 
> > Assume that the current value of top for a humongous object has not
> > been seen yet by the thread and we end up with an empty
> > intersection.
> > 
> > Now, didn't we potentially just drop a card to a humongous object
> > in waiting to scan but did not re-enqueue it? (And we did not clear
> > the card table value either?)
> > 
> > We may do it after the fence though I think.
> > 
> > Maybe I am completely wrong though, what do you think?
> If we see the old (zero) value of top in conjunction with a humongous
> region type, it is because this is a stale card.  If this were a
> non-stale card, the synchronization between enqueuing the card and
> reaching refinement would have ensured we see an up-to-date top (as
> well as an up-to-date type).  Card table entries for a free region
> are cleaned before the region can be allocated (and there are locks
> in the allocation path that provide the needed ordering).  Since this
> is a stale card and regions are allocated with clean card table
> entries, the dirty card table entry check having passed implies there
> is another (non-stale and not-yet-processed) card making its way to
> refinement through the usual channels, including the needed
> synchronization barriers.

Thanks. Again I was mostly worried about noting this reliance on
previous synchronization down somewhere, even if it is only the mailing
list.

It may be useful to note this in the code too. This would save the next
one working on this code looking through old mailing list threads.

Maybe I am a bit overly concerned about making sure that these thoughts
are provided in the proper place though. Or maybe everyone thinks that
everything is clear :)

> > 
> > - another stale comment:
> > 
> >  636   // a card beyond the heap.  This is not safe without a perm
> >  637   // gen at the upper end of the heap.
> > 
> > Could everything after "without" be removed in this sentence? We
> > haven't had a "perm gen" for a long time…
> Yes.  I’ll make that change.
> 

Thanks.

Thanks,
  Thomas