<div dir="ltr">Dear Hiroshi,<div><br></div><div>It looks like  psPromotionManager.cpp:509 contains a logging statement that could read data from an oop forwarded by another thread.</div><div><br></div><div>I don't see how your new logging in <span style="color:rgb(0,0,0)">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 (</span><font color="#000000">new_obj->klass() reads new_obj->_metadata).</font></div><div><br></div><div>Carsten</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Oct 2, 2016 at 10:46 AM, Hiroshi H Horii <span dir="ltr"><<a href="mailto:HORII@jp.ibm.com" target="_blank">HORII@jp.ibm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><font size="2" face="sans-serif">Hi, Thomas, and David,</font><span class=""><br><br><font size="2" face="sans-serif">Thank you for your comments.</font><br><br></span><span class=""><font size="2" face="sans-serif">> I think Hiroshi thinks that since
the work stealing itself does a CAS</font><br><font size="2" face="sans-serif">> with barrier after obtaining "new_obj"
in the other thread, it should</font><br><font size="2" face="sans-serif">> be safe (for other threads consuming
an object on the task queue).</font><br><br></span><font size="2" face="sans-serif">Thank you. What Thomas thankfully explain
is that I wanted to mention why relaxed CAS is available for copy_to_survivor.</font><span class=""><br><br><font size="2" face="sans-serif">> I also do not think it is safe
as is - for example, at least</font><br><font size="2" face="sans-serif">> PSPromotionManager::copy_and_<wbr>push_safe_barrier()
reads data from the</font><br><font size="2" face="sans-serif">> returned new_obj (in another log
message :)) regardless of failure.</font><br><font size="2" face="sans-serif">> </font><br><font size="2" face="sans-serif">> That method also reads the forwardee
if forwarded, and then again uses</font><br><font size="2" face="sans-serif">> object information in that same
log message. A quick look did not show</font><br><font size="2" face="sans-serif">> other issues, but don't count this
as a review.</font><br><br></span><span class=""><font size="2" face="sans-serif">Thank you for your comments.</font><br><br></span><font size="2" face="sans-serif">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.</font><br><br><font size="2" face="sans-serif">In my understanding, PSPromotionManager::copy_and_<wbr>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.</font><br><br><font size="2" face="sans-serif">I created a new webrev that reduces
print formats when CAS is failed. Could you review this and give comments
on it?</font><br><a href="http://cr.openjdk.java.net/~horii/8154736/webrev.00/" target="_blank"><font size="2" color="blue" face="sans-serif">http://cr.openjdk.java.net/~<wbr>horii/8154736/webrev.00/</font></a><span class=""><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></span><tt><font size="2">Thomas Schatzl <<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>> wrote
on 09/30/2016 21:02:31:<br><br>> From: Thomas Schatzl <<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>></font></tt><br><tt><font size="2">> To: David Holmes <<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a>>,
Hiroshi H Horii/Japan/IBM@IBMJP</font></tt><br><tt><font size="2">> Cc: hotspot-compiler-dev <<a href="mailto:hotspot-compiler-dev@openjdk.java.net" target="_blank">hotspot-compiler-dev@openjdk.<wbr>java.net</a>>,
<br>> Tim Ellison <<a href="mailto:Tim_Ellison@uk.ibm.com" target="_blank">Tim_Ellison@uk.ibm.com</a>>, Michihiro Horie/Japan/<br>> IBM@IBMJP, "<a href="mailto:ppc-aix-port-dev@openjdk.java.net" target="_blank">ppc-aix-port-dev@openjdk.<wbr>java.net</a>" <ppc-aix-port-<br>> <a href="mailto:dev@openjdk.java.net" target="_blank">dev@openjdk.java.net</a>>, "<a href="mailto:hotspot-gc-dev@openjdk.java.net" target="_blank">hotspot-gc-dev@openjdk.java.<wbr>net</a>"
<hotspot-<br>> <a href="mailto:gc-dev@openjdk.java.net" target="_blank">gc-dev@openjdk.java.net</a>>, "<a href="mailto:hotspot-runtime-dev@openjdk.java.net" target="_blank">hotspot-runtime-dev@openjdk.<wbr>java.net</a>"
<br>> <<a href="mailto:hotspot-runtime-dev@openjdk.java.net" target="_blank">hotspot-runtime-dev@openjdk.<wbr>java.net</a>></font></tt><br><tt><font size="2">> Date: 09/30/2016 21:04</font></tt><span class=""><br><tt><font size="2">> Subject: Re: RFR(M): 8154736: enhancement of
cmpxchg and <br>> copy_to_survivor for ppc64</font></tt><br></span><tt><font size="2">> <br><div><div class="h5">> 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/<wbr>parallel/psPromotionManager.<wbr>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/<wbr>parallel/psPromotionManager.<wbr>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_<wbr>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></div></div></font></tt><br></blockquote></div><br></div>