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

David Holmes david.holmes at oracle.com
Thu Oct 6 01:36:15 UTC 2016

On 5/10/2016 10:36 AM, Hiroshi H Horii wrote:

Dear David,

Thank you for your comments. I just used to think that it may be better that copytosurvivorspace 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/

Which simply takes us back to where we were. It may not be safe for the caller of those methods to access the fields of the returned "forwardee".

Sorry but I'm not seeing anything here that justifies removing the barriers from the cas in this code. GC lurkers feel free to jump in here

David

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 copytosurvivorspace > 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 newobj! >> >> 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::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 >> >> > >> >

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