<font size=2 face="sans-serif">Dear David,</font><br><br><font size=2 face="sans-serif">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. </font><br><br><font size=2 face="sans-serif">I changed the log format in copy_and_push_safe_barrier
not to use fields of new_obj. Could you review this again? </font><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.02/><font size=2 color=blue face="sans-serif">http://cr.openjdk.java.net/~horii/8154736/webrev.02/</font></a><br><br><font size=2 face="sans-serif">The callers of PSPromotionManager::copy_to_survivor_space
are here.</font><br><font size=2 face="sans-serif">  PSPromotionManager::copy_and_push_safe_barrier</font><br><font size=2 face="sans-serif">  PSScavengeFromKlassClosure::do_oop</font><br><br><font size=2 face="sans-serif">I confirmed any fields of new_obj is
not used in the two methods in this webrev. </font><br><br><font size=2 face="sans-serif">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.</font><br><br><font size=2 face="sans-serif">  if (log_develop_is_enabled(Trace,
gc, scavenge)) {<br>    log_develop_trace(gc, scavenge)(...);<br>  }<br></font><br><font size=2 face="sans-serif">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).</font><br><font size=2 face="sans-serif">  </font><br><font size=2 face="sans-serif">Regards,<br>Hiroshi<br>-----------------------<br>Hiroshi Horii, Ph.D.<br>IBM Research - Tokyo<br></font><br><br><tt><font size=2>David Holmes <david.holmes@oracle.com> wrote
on 10/04/2016 16:32:35:<br><br>> From: David Holmes <david.holmes@oracle.com></font></tt><br><tt><font size=2>> To: Hiroshi H Horii/Japan/IBM@IBMJP, Carsten
Varming <varming@gmail.com></font></tt><br><tt><font size=2>> Cc: hotspot-compiler-dev <hotspot-compiler-dev@openjdk.java.net>,
<br>> "hotspot-gc-dev@openjdk.java.net" <hotspot-gc-dev@openjdk.java.net>,<br>> "hotspot-runtime-dev@openjdk.java.net" <hotspot-runtime-<br>> dev@openjdk.java.net>, Michihiro Horie/Japan/IBM@IBMJP, "ppc-aix-<br>> port-dev@openjdk.java.net" <ppc-aix-port-dev@openjdk.java.net>,
<br>> Thomas Schatzl <thomas.schatzl@oracle.com>, Tim Ellison <br>> <Tim_Ellison@uk.ibm.com></font></tt><br><tt><font size=2>> Date: 10/04/2016 16:33</font></tt><br><tt><font size=2>> Subject: Re: RFR(M): 8154736: enhancement of
cmpxchg and <br>> copy_to_survivor for ppc64</font></tt><br><tt><font size=2>> <br>> On 4/10/2016 12:15 AM, Hiroshi H Horii wrote:<br>> > Dear Carsten,<br>> ><br>> > Thank you for your correction. And very sorry about my easy mistakes...<br>> > I created webrev again. </font></tt><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.01/><tt><font size=2>http://cr.openjdk.java.net/~horii/8154736/webrev.01/</font></tt></a><tt><font size=2><br>> > I believe, all of the unsafe usages of new_obj, which has been
pointed<br>> > in this thread, is fixed with this webrev.<br>> <br>> I still am uneasy about this. If it is not safe to access the fields
of <br>> new_obj in the tracing statements but we return new_obj to the caller,
<br>> then it may not be safe for the caller to access the fields of new_obj!<br>> <br>> That aside:<br>> <br>> src/share/vm/gc/parallel/psPromotionManager.inline.hpp<br>> <br>>   293   if (o->is_forwarded()) {<br>>   294     new_obj = o->forwardee();<br>>   295     // fields in new_obj may not be synchronized.<br>>   296     if (log_develop_is_enabled(Trace, gc, scavenge)
&& <br>> o->is_forwarded()) {<br>> <br>> Why the second check of o->is_forwarded() ?<br>> <br>> 297       log_develop_trace(gc, scavenge)("{%s
%s " PTR_FORMAT " -> " <br>> PTR_FORMAT "}",<br>>   298                
        "forwarding",<br>> <br>> Why are you passing "forwarding" as an argument for the
first %s instead <br>> of just expressing it directly? I see this is a copy'n'paste from
the <br>> existing code - and I'm guessing at one point there was a conditional
<br>> around that. I think it should be fixed.<br>> <br>> Thanks,<br>> David<br>> <br>> > Dear all,<br>> ><br>> > Can I ask a review of this webrev and give thoughts and comments
again?<br>> ><br>> > Regards,<br>> > Hiroshi<br>> > -----------------------<br>> > Hiroshi Horii, Ph.D.<br>> > IBM Research - Tokyo<br>> ><br>> ><br>> > Carsten Varming <varming@gmail.com> wrote on 10/03/2016
12:55:25:<br>> ><br>> >> From: Carsten Varming <varming@gmail.com><br>> >> To: Hiroshi H Horii/Japan/IBM@IBMJP<br>> >> Cc: Thomas Schatzl <thomas.schatzl@oracle.com>, David
Holmes<br>> >> <david.holmes@oracle.com>, hotspot-compiler-dev <hotspot-compiler-<br>> >> dev@openjdk.java.net>, "hotspot-gc-dev@openjdk.java.net"
<hotspot-<br>> >> gc-dev@openjdk.java.net>, "hotspot-runtime-dev@openjdk.java.net"<br>> >> <hotspot-runtime-dev@openjdk.java.net>, Michihiro Horie/Japan/<br>> >> IBM@IBMJP, "ppc-aix-port-dev@openjdk.java.net"
<ppc-aix-port-<br>> >> dev@openjdk.java.net>, Tim Ellison <Tim_Ellison@uk.ibm.com><br>> >> Date: 10/03/2016 12:56<br>> >> Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and<br>> >> copy_to_survivor for ppc64<br>> >><br>> >> Dear Hiroshi,<br>> >><br>> >> It looks like  psPromotionManager.cpp:509 contains a
logging<br>> >> statement that could read data from an oop forwarded by another
thread.<br>> >><br>> >> I don't see how your new logging<br>> >> in PSPromotionManager::copy_and_push_safe_barrier can be
safe. In<br>> >> the two new statements you read data from new_obj, but in
both cases<br>> >> it is possible that another thread still haven't written
the data in<br>> >> new_obj (new_obj->klass() reads new_obj->_metadata).<br>> >><br>> >> Carsten<br>> >><br>> >> On Sun, Oct 2, 2016 at 10:46 AM, Hiroshi H Horii <HORII@jp.ibm.com>
wrote:<br>> >> Hi, Thomas, and David,<br>> >><br>> >> Thank you for your comments.<br>> >><br>> >> > I think Hiroshi thinks that since the work stealing
itself does a CAS<br>> >> > with barrier after obtaining "new_obj" in
the other thread, it should<br>> >> > be safe (for other threads consuming an object on the
task queue).<br>> >><br>> >> Thank you. What Thomas thankfully explain is that I wanted
to<br>> >> mention why relaxed CAS is available for copy_to_survivor.<br>> >><br>> >> > I also do not think it is safe as is - for example,
at least<br>> >> > PSPromotionManager::copy_and_push_safe_barrier() reads
data from the<br>> >> > returned new_obj (in another log message :)) regardless
of failure.<br>> >> ><br>> >> > That method also reads the forwardee if forwarded, and
then again uses<br>> >> > object information in that same log message. A quick
look did not show<br>> >> > other issues, but don't count this as a review.<br>> >><br>> >> Thank you for your comments.<br>> >><br>> >> As Carsten suggested, I guess, size may not be necessary
for logging<br>> >> when CAS is failed (the size will be logged by the other
thread that<br>> >> successfully operates the CAS). By reducing printing a size
of<br>> >> new_obj, relaxing CAS for forwarding pointers becomes safe,
I believe.<br>> >><br>> >> In my understanding, PSPromotionManager::copy_and_push_safe_barrier<br>> >> () updates a card table for new_obj. However, this new_obj
will not<br>> >> be used fro card tables in the same GC as a root of GC because
all<br>> >> of entries in card tables were registered as tasks before
any calls<br>> >> of copy_and_push_safe_barrier.<br>> >><br>> >> I created a new webrev that reduces print formats when CAS
is<br>> >> failed. Could you review this and give comments on it?<br>> >> </font></tt><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.00/><tt><font size=2>http://cr.openjdk.java.net/~horii/8154736/webrev.00/</font></tt></a><tt><font size=2><br>> >><br>> >> Regards,<br>> >> Hiroshi<br>> >> -----------------------<br>> >> Hiroshi Horii, Ph.D.<br>> >> IBM Research - Tokyo<br>> >><br>> >><br>> >> Thomas Schatzl <thomas.schatzl@oracle.com> wrote on
09/30/2016 21:02:31:<br>> >><br>> >> > From: Thomas Schatzl <thomas.schatzl@oracle.com><br>> >> > To: David Holmes <david.holmes@oracle.com>, Hiroshi
H<br>> > Horii/Japan/IBM@IBMJP<br>> >> > Cc: hotspot-compiler-dev <hotspot-compiler-dev@openjdk.java.net>,<br>> >> > Tim Ellison <Tim_Ellison@uk.ibm.com>, Michihiro
Horie/Japan/<br>> >> > IBM@IBMJP, "ppc-aix-port-dev@openjdk.java.net"
<ppc-aix-port-<br>> >> > dev@openjdk.java.net>, "hotspot-gc-dev@openjdk.java.net"
<hotspot-<br>> >> > gc-dev@openjdk.java.net>, "hotspot-runtime-dev@openjdk.java.net"<br>> >> > <hotspot-runtime-dev@openjdk.java.net><br>> >> > Date: 09/30/2016 21:04<br>> >> > Subject: Re: RFR(M): 8154736: enhancement of cmpxchg
and<br>> >> > copy_to_survivor for ppc64<br>> >> ><br>> >> > Hi,<br>> >> ><br>> >> > On Fri, 2016-09-30 at 21:12 +1000, David Holmes wrote:<br>> >> > > On 30/09/2016 8:17 PM, Hiroshi H Horii wrote:<br>> >> > > ><br>> >> > > > Dear David, and Dan,<br>> >> > > ><br>> >> > > > Thank you for your comments.<br>> >> > > ><br>> >> > > > ><br>> >> > > > > In<br>> >> > > > > hotspot/src/share/vm/gc/parallel/psPromotionManager.inline.hpp:<br>> >> > > > > 266 the log line reads data from the
forwardee even when the CAS<br>> >> > > > > fails. I believe those reads will be
unsafe without barriers<br>> >> > > > > after<br>> >> > > > > the copy of the content of the object.<br>> >> > > > > hotspot/src/share/vm/gc/parallel/psPromotionManager.inline.hpp:28<br>> >> > > > > 8<br>> >> > > > > same problem as in line 266<br>> >> > > > Can we use o->size() or new_obj_size instead
of new_obj->size()?<br>> >> ><br>> >> > They are not equivalent. Parallel GC and other collectors
creatively<br>> >> > reuse the "length" field of objArrays to indicate
progress in the<br>> >> > scanning them during GC.<br>> >> ><br>> >> > new_obj_size is the result of a call to o->size()
(and the compiler may<br>> >> > redo computations at any point), so has the same issue.<br>> >> ><br>> >> > > > > If you feel that the use of new_obj->size()
is potentially unsafe<br>> >> > > > > then<br>> >> > > > > the fact we return new_obj means that
any use of new_obj by the<br>> >> > > > > caller<br>> >> > > > > may also potentially be unsafe.<br>> >> > > > In my understanding, while copying objects
to a survivor space, if<br>> >> > > > a thread creates a new_obj and sets a pointer
with CAS, the other<br>> >> > > > threads can touch the new_obj after the thread
calls<br>> >> > > > push_contents(new_obj) (Line: 239). In push_contents,<br>> >> > > > OrderAccess::release_store is called before
pushing the object as a<br>> >> > > > task into a deque of workstealing (taskqueue.inline.hpp).
If the<br>> >> > > > other thread reads the task, all of copy for
new_obj is safe.<br>> >> > > I'm not familiar with the larger picture of the
GC protocols here,<br>> >> > > but just looking at this code fragment in isolation
if the CAS fails<br>> >> > > we read o->forwardee() to set new_obj. That
in itself is fine because<br>> >> > > we're reading the field that we were testing with
the CAS. But we<br>> >> > > could then deference new_obj before the thread
that won the CAS calls<br>> >> > > push_contents; and even if it is after push_contents
we have not done<br>> >> > > an acquire to pair with the release-store in push_contents.<br>> >> ><br>> >> > I think Hiroshi thinks that since the work stealing
itself does a CAS<br>> >> > with barrier after obtaining "new_obj" in
the other thread, it should<br>> >> > be safe (for other threads consuming an object on the
task queue).<br>> >> ><br>> >> > > So I'm really not seeing how we can use a barrier-less
CAS here.<br>> >> ><br>> >> > I also do not think it is safe as is - for example,
at least<br>> >> > PSPromotionManager::copy_and_push_safe_barrier() reads
data from the<br>> >> > returned new_obj (in another log message :)) regardless
of failure.<br>> >> ><br>> >> > That method also reads the forwardee if forwarded, and
then again uses<br>> >> > object information in that same log message. A quick
look did not show<br>> >> > other issues, but don't count this as a review.<br>> >> ><br>> >> > Thanks,<br>> >> >   Thomas<br>> >> ><br>> <br></font></tt><BR>