RFR(M): 8154736: enhancement of cmpxchg and copy_to_survivor for ppc64

[Thomas Schatzl]

Hi,

On Fri, 2016-09-30 at 21:12 +1000, David Holmes wrote:
> On 30/09/2016 8:17 PM, Hiroshi H Horii wrote:
> > 
> > Dear David, and Dan,
> > 
> > Thank you for your comments.
> > 
> > > 
> > > In
> > > hotspot/src/share/vm/gc/parallel/psPromotionManager.inline.hpp:
> > > 266 the log line reads data from the forwardee even when the CAS
> > > fails. I believe those reads will be unsafe without barriers
> > > after
> > > the copy of the content of the object.
> > > hotspot/src/share/vm/gc/parallel/psPromotionManager.inline.hpp:28
> > > 8
> > > same problem as in line 266
> > Can we use o->size() or new_obj_size instead of new_obj->size()?

They are not equivalent. Parallel GC and other collectors creatively
reuse the "length" field of objArrays to indicate progress in the
scanning them during GC.

new_obj_size is the result of a call to o->size() (and the compiler may
redo computations at any point), so has the same issue.

> > > If you feel that the use of new_obj->size() is potentially unsafe
> > > then
> > > the fact we return new_obj means that any use of new_obj by the
> > > caller
> > > may also potentially be unsafe.
> > In my understanding, while copying objects to a survivor space, if
> > a thread creates a new_obj and sets a pointer with CAS, the other
> > threads can touch the new_obj after the thread calls
> > push_contents(new_obj) (Line: 239). In push_contents,
> > OrderAccess::release_store is called before pushing the object as a
> > task into a deque of workstealing (taskqueue.inline.hpp). If the
> > other thread reads the task, all of copy for new_obj is safe.
> I'm not familiar with the larger picture of the GC protocols here,
> but just looking at this code fragment in isolation if the CAS fails
> we read o->forwardee() to set new_obj. That in itself is fine because
> we're reading the field that we were testing with the CAS. But we
> could then deference new_obj before the thread that won the CAS calls
> push_contents; and even if it is after push_contents we have not done
> an acquire to pair with the release-store in push_contents.

I think Hiroshi thinks that since the work stealing itself does a CAS
with barrier after obtaining "new_obj" in the other thread, it should
be safe (for other threads consuming an object on the task queue).

> So I'm really not seeing how we can use a barrier-less CAS here.

I also do not think it is safe as is - for example, at least
PSPromotionManager::copy_and_push_safe_barrier() reads data from the
returned new_obj (in another log message :)) regardless of failure.

That method also reads the forwardee if forwarded, and then again uses
object information in that same log message. A quick look did not show
other issues, but don't count this as a review.

Thanks,
  Thomas