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

Hiroshi H Horii HORII at jp.ibm.com
Wed Oct 5 00:36:37 UTC 2016

Dear David,

Thank you for your comments.

I just used to think that it may be better that copy_to_survivor_space doesn't return forwardee if CAS was failed in order to prevent from reading fields in forwardee. But as you pointed, this extends fix for this topic.

I removed two NULL assignments from the previous wevrev. http://cr.openjdk.java.net/~horii/8154736/webrev.03/

Thank you for reviewing multiple times...

Regards, Hiroshi

Hiroshi Horii, Ph.D. IBM Research - Tokyo

David Holmes <david.holmes at oracle.com> wrote on 10/04/2016 21:16:33:

From: David Holmes <david.holmes at oracle.com> To: Hiroshi H Horii/Japan/IBM at IBMJP Cc: hotspot-compiler-dev <hotspot-compiler-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>, Michihiro Horie/Japan/IBM at IBMJP, "ppc-aix- port-dev at openjdk.java.net" <ppc-aix-port-dev at openjdk.java.net>, Thomas Schatzl <thomas.schatzl at oracle.com>, Tim Ellison <TimEllison at uk.ibm.com>, Carsten Varming <varming at gmail.com> Date: 10/04/2016 21:17 Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and copytosurvivor for ppc64

On 4/10/2016 8:22 PM, Hiroshi H Horii wrote: > Dear David, > > Thank you for your comments. You are correct. In the previous webrev, a > caller (in copyandpushsafebarrier) may use newobj's fields > unsafely. Very sorry. > > I changed the log format in copyandpushsafebarrier not to use fields > of newobj. Could you review this again? > http://cr.openjdk.java.net/~horii/8154736/webrev.02/ src/share/vm/gc/parallel/psPromotionManager.inline.hpp 274 newobj = NULL; 285 newobj = NULL; Sorry but you are losing me here. You've gone from simply removing barriers on the cmpxchg to changing the functionality of the methods that use the cmpxchg - instead of return the forwardee() you are now returning NULL! ?? David ----- > The callers of PSPromotionManager::copytosurvivorspace are here. > PSPromotionManager::copyandpushsafebarrier > PSScavengeFromKlassClosure::dooop > > I confirmed any fields of newobj is not used in the two methods in this > webrev. > > In addition, I reduced passing a constant literal "forwarding" in > copyandpushsafebarrier and added some guards before logging in > PSPromotionManager::copytosurvivorspace as follows. > > if (logdevelopisenabled(Trace, gc, scavenge)) { > logdeveloptrace(gc, scavenge)(...); > } > > If copytosurvivorspace should not return newobj if its fields are > unsafe, I would like to change the return type of copy_to_survivor_space > to "void" (or allow copytosurvivorspace to return NULL). > > Regards, > Hiroshi > ----------------------- > Hiroshi Horii, Ph.D. > IBM Research - Tokyo > > > David Holmes <david.holmes at oracle.com> wrote on 10/04/2016 16:32:35: > >> From: David Holmes <david.holmes at oracle.com> >> To: Hiroshi H Horii/Japan/IBM at IBMJP, Carsten Varming <varming at gmail.com> >> Cc: hotspot-compiler-dev <hotspot-compiler-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" > dev at openjdk.java.net>, Michihiro Horie/Japan/IBM at IBMJP, "ppc-aix- >> port-dev at openjdk.java.net" <ppc-aix-port-dev at openjdk.java.net>, >> Thomas Schatzl <thomas.schatzl at oracle.com>, Tim Ellison >> <TimEllison at uk.ibm.com> >> Date: 10/04/2016 16:33 >> Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and >> copytosurvivor for ppc64 >> >> On 4/10/2016 12:15 AM, Hiroshi H Horii wrote: >> > Dear Carsten, >> > >> > Thank you for your correction. And very sorry about my easy mistakes... >> > I created webrev again. > http://cr.openjdk.java.net/~horii/8154736/webrev.01/ >> > I believe, all of the unsafe usages of newobj, which has been pointed >> > in this thread, is fixed with this webrev. >> >> I still am uneasy about this. If it is not safe to access the fields of >> newobj in the tracing statements but we return newobj to the caller, >> then it may not be safe for the caller to access the fields of new_obj! >> >> That aside: >> >> src/share/vm/gc/parallel/psPromotionManager.inline.hpp >> >> 293 if (o->isforwarded()) { >> 294 newobj = o->forwardee(); >> 295 // fields in newobj may not be synchronized. >> 296 if (logdevelopisenabled(Trace, gc, scavenge) && >> o->isforwarded()) { >> >> Why the second check of o->isforwarded() ? >> >> 297 logdeveloptrace(gc, scavenge)("{%s %s " PTRFORMAT " -> " >> PTRFORMAT "}", >> 298 "forwarding", >> >> Why are you passing "forwarding" as an argument for the first %s instead >> of just expressing it directly? I see this is a copy'n'paste from the >> existing code - and I'm guessing at one point there was a conditional >> around that. I think it should be fixed. >> >> Thanks, >> David >> >> > Dear all, >> > >> > Can I ask a review of this webrev and give thoughts and comments again? >> > >> > Regards, >> > Hiroshi >> > ----------------------- >> > Hiroshi Horii, Ph.D. >> > IBM Research - Tokyo >> > >> > >> > Carsten Varming <varming at gmail.com> wrote on 10/03/2016 12:55:25: >> > >> >> From: Carsten Varming <varming at gmail.com> >> >> To: Hiroshi H Horii/Japan/IBM at IBMJP >> >> Cc: Thomas Schatzl <thomas.schatzl at oracle.com>, David Holmes >> >> <david.holmes at oracle.com>, hotspot-compiler-dev > >> 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>, Michihiro Horie/Japan/ >> >> IBM at IBMJP, "ppc-aix-port-dev at openjdk.java.net" > >> dev at openjdk.java.net>, Tim Ellison <TimEllison at uk.ibm.com> >> >> Date: 10/03/2016 12:56 >> >> Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and >> >> copytosurvivor for ppc64 >> >> >> >> 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::copyandpushsafebarrier can be safe. In >> >> the two new statements you read data from newobj, but in both cases >> >> it is possible that another thread still haven't written the data in >> >> newobj (newobj->klass() reads newobj->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::copy_and_push_safe_barrier >> >> () 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 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. >> >> > >> >> > 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/20161005/7ab58d55/attachment-0001.html>

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