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

Carsten Varming varming at gmail.com
Mon Oct 3 03:55:25 UTC 2016

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 "newobj" 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 copytosurvivor. > I also do not think it is safe as is - for example, at least > PSPromotionManager::copyandpushsafebarrier() reads data from the > returned newobj (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 newobj, relaxing CAS for forwarding pointers becomes safe, I believe. In my understanding, PSPromotionManager::copyandpushsafebarrier() updates a card table for newobj. However, this newobj 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 copyandpushsafebarrier. 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 <TimEllison at uk.ibm.com>, Michihiro Horie/Japan/ > IBM at IBMJP, "ppc-aix-port-dev at openjdk.java.net" dev at openjdk.java.net>, "hotspot-gc-dev at openjdk.java.net" 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 > copytosurvivor 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 newobjsize instead of newobj->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. > > newobjsize 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 newobj->size() is potentially unsafe > > > > then > > > > the fact we return newobj means that any use of newobj by the > > > > caller > > > > may also potentially be unsafe. > > > In my understanding, while copying objects to a survivor space, if > > > a thread creates a newobj and sets a pointer with CAS, the other > > > threads can touch the newobj after the thread calls > > > pushcontents(newobj) (Line: 239). In pushcontents, > > > OrderAccess::releasestore 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 newobj 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 newobj. That in itself is fine because > > we're reading the field that we were testing with the CAS. But we > > could then deference newobj before the thread that won the CAS calls > > pushcontents; and even if it is after pushcontents we have not done > > an acquire to pair with the release-store in pushcontents. > > I think Hiroshi thinks that since the work stealing itself does a CAS > with barrier after obtaining "newobj" 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::copyandpushsafebarrier() reads data from the > returned newobj (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: <http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20161002/0af57cdb/attachment-0001.html>

More information about the ppc-aix-port-dev mailing list