<font size=2 face="sans-serif">Dear David,</font><br><br><font size=2 face="sans-serif">Thank you for your comments.</font><br><br><font size=2 face="sans-serif">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. </font><br><br><font size=2 face="sans-serif">I removed two NULL assignments from
the previous wevrev. </font><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.03/><font size=2 color=blue face="sans-serif">http://cr.openjdk.java.net/~horii/8154736/webrev.03/</font></a><br><br><font size=2 face="sans-serif">Thank you for reviewing multiple times...</font><br><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 21:16:33:<br><br>> From: David Holmes <david.holmes@oracle.com></font></tt><br><tt><font size=2>> To: Hiroshi H Horii/Japan/IBM@IBMJP</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>, Carsten Varming <varming@gmail.com></font></tt><br><tt><font size=2>> Date: 10/04/2016 21:17</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 8:22 PM, Hiroshi H Horii wrote:<br>> > Dear David,<br>> ><br>> > Thank you for your comments. You are correct. In the previous
webrev, a<br>> > caller (in copy_and_push_safe_barrier) may use new_obj's fields<br>> > unsafely. Very sorry.<br>> ><br>> > I changed the log format in copy_and_push_safe_barrier not to
use fields<br>> > of new_obj. Could you review this again?<br>> > </font></tt><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.02/><tt><font size=2>http://cr.openjdk.java.net/~horii/8154736/webrev.02/</font></tt></a><tt><font size=2><br>> <br>> src/share/vm/gc/parallel/psPromotionManager.inline.hpp<br>> <br>> 274       new_obj = NULL;<br>> 285     new_obj = NULL;<br>> <br>> Sorry but you are losing me here. You've gone from simply removing
<br>> barriers on the cmpxchg to changing the functionality of the methods
<br>> that use the cmpxchg - instead of return the forwardee() you are now
<br>> returning NULL! ??<br>> <br>> David<br>> -----<br>> <br>> > The callers of PSPromotionManager::copy_to_survivor_space are
here.<br>> >   PSPromotionManager::copy_and_push_safe_barrier<br>> >   PSScavengeFromKlassClosure::do_oop<br>> ><br>> > I confirmed any fields of new_obj is not used in the two methods
in this<br>> > webrev.<br>> ><br>> > In addition, I reduced passing a constant literal "forwarding"
in<br>> > copy_and_push_safe_barrier and added some guards before logging
in<br>> > PSPromotionManager::copy_to_survivor_space as follows.<br>> ><br>> >   if (log_develop_is_enabled(Trace, gc, scavenge)) {<br>> >    log_develop_trace(gc, scavenge)(...);<br>> >  }<br>> ><br>> > If copy_to_survivor_space should not return new_obj if its fields
are<br>> > unsafe, I would like to change the return type of copy_to_survivor_space<br>> > to "void" (or allow copy_to_survivor_space to return
NULL).<br>> ><br>> > Regards,<br>> > Hiroshi<br>> > -----------------------<br>> > Hiroshi Horii, Ph.D.<br>> > IBM Research - Tokyo<br>> ><br>> ><br>> > David Holmes <david.holmes@oracle.com> wrote on 10/04/2016
16:32:35:<br>> ><br>> >> From: David Holmes <david.holmes@oracle.com><br>> >> To: Hiroshi H Horii/Japan/IBM@IBMJP, Carsten Varming <varming@gmail.com><br>> >> 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><br>> >> Date: 10/04/2016 16:33<br>> >> Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and<br>> >> copy_to_survivor for ppc64<br>> >><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.<br>> > </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><br>> > 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<br>> > uses<br>> >> >> > object information in that same log message.
A quick look did not<br>> > 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<br>> > 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<br>> > 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>> >> >> > > > ><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<br>> > 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<br>> > 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<br>> > 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<br>> > 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<br>> > fails<br>> >> >> > > we read o->forwardee() to set new_obj.
That in itself is fine<br>> > 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<br>> > calls<br>> >> >> > > push_contents; and even if it is after
push_contents we have<br>> > 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<br>> > uses<br>> >> >> > object information in that same log message.
A quick look did not<br>> > show<br>> >> >> > other issues, but don't count this as a review.<br>> >> >> ><br>> >> >> > Thanks,<br>> >> >> >   Thomas<br>> >> >> ><br>> >><br>> ><br>> <br></font></tt><BR>