<font size=2 face="sans-serif">Dear Carsten,</font><br><br><font size=2 face="sans-serif">Thank you for your correction. And very

sorry about my easy mistakes...</font><br><font size=2 face="sans-serif">I created webrev again. </font><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.01/><font size=2 color=blue face="sans-serif">http://cr.openjdk.java.net/~horii/8154736/webrev.01/</font></a><br><font size=2 face="sans-serif">I believe, all of the unsafe usages

of new_obj, which has been pointed in this thread, is fixed with this webrev.</font><br><br><font size=2 face="sans-serif">Dear all,</font><br><br><font size=2 face="sans-serif">Can I ask a review of this webrev and

give thoughts and comments again?</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>Carsten Varming <varming@gmail.com> wrote on

10/03/2016 12:55:25:<br><br>> From: Carsten Varming <varming@gmail.com></font></tt><br><tt><font size=2>> To: Hiroshi H Horii/Japan/IBM@IBMJP</font></tt><br><tt><font size=2>> 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></font></tt><br><tt><font size=2>> Date: 10/03/2016 12:56</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>> Dear Hiroshi,</font></tt><br><tt><font size=2>> <br>> It looks like  psPromotionManager.cpp:509 contains a logging

> statement that could read data from an oop forwarded by another thread.</tt> <tt><font size=2>>  > I don't see how your new logging  > 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).</font></tt><br><tt><font size=2>> <br>> Carsten</font></tt><br><tt><font size=2>> <br>> On Sun, Oct 2, 2016 at 10:46 AM, Hiroshi H Horii <HORII@jp.ibm.com>

wrote:</font></tt><br><tt><font size=2>> 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 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>> > </font></tt><br><tt><font size=2>> > 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 > > > > 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 > > > 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>> > </font></tt><BR>