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

Hiroshi H Horii HORII at jp.ibm.com
Tue Oct 4 10:22:49 UTC 2016


Dear David,

Thank you for your comments. You are correct. In the previous webrev, a 
caller (in copy_and_push_safe_barrier) may use new_obj's fields unsafely. 
Very sorry. 

I changed the log format in copy_and_push_safe_barrier not to use fields 
of new_obj. Could you review this again? 
http://cr.openjdk.java.net/~horii/8154736/webrev.02/

The callers of PSPromotionManager::copy_to_survivor_space are here.
  PSPromotionManager::copy_and_push_safe_barrier
  PSScavengeFromKlassClosure::do_oop

I confirmed any fields of new_obj is not used in the two methods in this 
webrev. 

In addition, I reduced passing a constant literal "forwarding" in 
copy_and_push_safe_barrier and added some guards before logging in 
PSPromotionManager::copy_to_survivor_space as follows.

  if (log_develop_is_enabled(Trace, gc, scavenge)) {
    log_develop_trace(gc, scavenge)(...);
  }

If copy_to_survivor_space should not return new_obj if its fields are 
unsafe, I would like to change the return type of copy_to_survivor_space 
to "void" (or allow copy_to_survivor_space 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" <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 
> <Tim_Ellison at uk.ibm.com>
> Date: 10/04/2016 16:33
> Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and 
> copy_to_survivor 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 new_obj, 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 
> new_obj in the tracing statements but we return new_obj 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->is_forwarded()) {
>   294     new_obj = o->forwardee();
>   295     // fields in new_obj may not be synchronized.
>   296     if (log_develop_is_enabled(Trace, gc, scavenge) && 
> o->is_forwarded()) {
> 
> Why the second check of o->is_forwarded() ?
> 
> 297       log_develop_trace(gc, scavenge)("{%s %s " PTR_FORMAT " -> " 
> PTR_FORMAT "}",
>   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 <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>, Tim Ellison <Tim_Ellison at uk.ibm.com>
> >> Date: 10/03/2016 12:56
> >> Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and
> >> copy_to_survivor 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::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.java.net/pipermail/hotspot-gc-dev/attachments/20161004/8f28fb2c/attachment.htm>


More information about the hotspot-gc-dev mailing list