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

[Carsten Varming]

Dear Hiroshi,

It looks like  psPromotionManager.cpp:509 contains a logging statement that
could read data from an oop forwarded by another thread.

I don't see how your new logging in
PSPromotionManager::copy_and_push_safe_barrier
can be safe. In the two new statements you read data from new_obj, but in
both cases it is possible that another thread still haven't written the
data in new_obj (new_obj->klass() reads new_obj->_metadata).

Carsten

On Sun, Oct 2, 2016 at 10:46 AM, Hiroshi H Horii <HORII at jp.ibm.com> wrote:

> Hi, Thomas, and David,
>
> Thank you for your comments.
>
> > 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).
>
> Thank you. What Thomas thankfully explain is that I wanted to mention why
> relaxed CAS is available for copy_to_survivor.
>
> > 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.
>
> Thank you for your comments.
>
> As Carsten suggested, I guess, size may not be necessary for logging when
> CAS is failed (the size will be logged by the other thread that
> successfully operates the CAS). By reducing printing a size of new_obj,
> relaxing CAS for forwarding pointers becomes safe, I believe.
>
> In my understanding, PSPromotionManager::copy_and_push_safe_barrier()
> updates a card table for new_obj. However, this new_obj will not be used
> fro card tables in the same GC as a root of GC because all of entries in
> card tables were registered as tasks before any calls of
> copy_and_push_safe_barrier.
>
> I created a new webrev that reduces print formats when CAS is failed.
> Could you review this and give comments on it?
> http://cr.openjdk.java.net/~horii/8154736/webrev.00/
>
> Regards,
> Hiroshi
> -----------------------
> Hiroshi Horii, Ph.D.
> IBM Research - Tokyo
>
>
> Thomas Schatzl <thomas.schatzl at oracle.com> wrote on 09/30/2016 21:02:31:
>
> > From: Thomas Schatzl <thomas.schatzl at oracle.com>
> > To: David Holmes <david.holmes at oracle.com>, Hiroshi H
> Horii/Japan/IBM at IBMJP
> > Cc: hotspot-compiler-dev <hotspot-compiler-dev at openjdk.java.net>,
> > Tim Ellison <Tim_Ellison at uk.ibm.com>, Michihiro Horie/Japan/
> > IBM at IBMJP, "ppc-aix-port-dev at openjdk.java.net" <ppc-aix-port-
> > dev at openjdk.java.net>, "hotspot-gc-dev at openjdk.java.net" <hotspot-
> > gc-dev at openjdk.java.net>, "hotspot-runtime-dev at openjdk.java.net"
> > <hotspot-runtime-dev at openjdk.java.net>
> > Date: 09/30/2016 21:04
> > Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and
> > copy_to_survivor for ppc64
> >
> > 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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20161002/0af57cdb/attachment.htm>