<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Chris,<br>
<br>
Please, let me know if you have more thoughts on this or it is
okay to push as it is.<br>
And thank you a lot for reviewing this!<br>
<br>
Thanks,<br>
Serguei <br>
<br>
<br>
On 10/8/19 13:20, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:dd97ce26-9853-07f8-d3b5-ea3d26d83a48@oracle.com"> Hi
Chris,<br>
<br>
<div class="moz-cite-prefix">On 10/8/19 12:56 PM, Chris Plummer
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6d484361-9e5b-3d05-8a23-bcf391992839@oracle.com">
<div class="moz-cite-prefix">Hi Serguei,<br>
<br>
The just seems like an odd coding pattern to me:<br>
<br>
// Block until the suspender thread competes the tested
threads suspension<br>
agent_lock(jni);<br>
agent_unlock(jni);<br>
</div>
</blockquote>
<br>
I do not consider it as odd but normal.<br>
We have similar coding patterns in the jdwp agent.<br>
The agent_unlock(jni) call can be moved to the end of the function
if you like.<br>
It does not matter in this particular case but will look "better".<br>
<br>
<blockquote type="cite"
cite="mid:6d484361-9e5b-3d05-8a23-bcf391992839@oracle.com">
<div class="moz-cite-prefix"> Wouldn't you normally use
wait/notify in this situation? Some like:<br>
<br>
agent_lock(jni);<br>
if (!suspenderDone) {<br>
agent_wait(jni);<br>
}<br>
agent_unlock(jni);<br>
</div>
</blockquote>
<br>
The above will make synchronization more complex.
<blockquote type="cite"
cite="mid:6d484361-9e5b-3d05-8a23-bcf391992839@oracle.com">
<div class="moz-cite-prefix"> <br>
I think maybe you left out the middle part because you know
that this code will never grab the agent_lock before the
suspender thread does, and therefore once this code gets the
lock you know the suspender thread is done (is that actually
the case?).<br>
</div>
</blockquote>
Yes, it is the case.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<blockquote type="cite"
cite="mid:6d484361-9e5b-3d05-8a23-bcf391992839@oracle.com">
<div class="moz-cite-prefix"> thanks,<br>
<br>
Chris<br>
<br>
On 10/8/19 11:50 AM, <a class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:bbbd93a0-fcdf-b840-1297-5818139d2725@oracle.com">
Ping...<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<div class="moz-cite-prefix">On 10/7/19 5:25 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4ca3e8f8-0b6d-c94b-44d8-45661ed24f49@oracle.com">
<div class="moz-cite-prefix">On 10/7/19 17:14, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:01e82088-3ea0-8af8-840e-c4c9a6bc52c1@oracle.com">
Hi Alex, Chris and David,<br>
<br>
The mach5 testing in 100 runs loop on all platform
discovered a race in new test.<br>
It is between the native suspendTestedThreads() called on
the suspender thread<br>
and the checkSuspendedStatus() calling native
checkTestedThreadsSuspended().<br>
<br>
It occurred that the iteration count and sleep time in
checkSuspendedStatus() can be not enough:<br>
<pre> 100 private boolean checkSuspendedStatus(ThreadToSuspend[] threads) throws RuntimeException {
101 boolean success = false;
102
103 log("Main: checking all tested threads have been suspended");
104
105 // wait for tested threads to reach suspended status
106 for (int i = 0; !success && i < 20; i++) {
107 try {
108 Thread.sleep(10);
109 } catch (InterruptedException e) {
110 throw new RuntimeException("Main: sleep was interrupted\n\t" + e);
111 }
112 success = checkTestedThreadsSuspended();
113 }
114 return success;
115 }</pre>
<br>
So, I've decided to refactor/update the test a little bit:
<br>
<br>
1. Now the checkSuspendedStatus() is:<br>
<pre> 100 private boolean checkSuspendedStatus() throws RuntimeException {
101 log("Main: checking all tested threads have been suspended");
102 return checkTestedThreadsSuspended();
103 }</pre>
2. A raw monitor agent_monitor is added to the native
agent.<br>
It is created and locked in the
Java_ThreadToSuspend_init() function called at<br>
the beginning of the ThreadToSuspend.run() execution of
the suspender thread<br>
and unlocked at the end of
Java_ThreadToSuspend_suspendTestedThreads().<br>
<br>
The
Java_SuspendWithCurrentThread_checkTestedThreadsSuspended()
grabs the agent_monitor<br>
on the Main thread to wait until the
suspendTestedThreads() completes its work.<br>
<br>
3. Also, the results[] array is always created and used
locally in the functions<br>
where it is needed: suspendTestedThreads() and
resumeTestedThreads().<br>
</blockquote>
<br>
Forgot to tell about one more change:<br>
<br>
4. The releaseTestedThreads() is renamed to
releaseTestedThreadsInfo() and moved<br>
to the end of the Main thread run() method.<br>
<br>
Also wanted to thank Alex for sharing good suggestions on
the sync approaches. <br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:01e82088-3ea0-8af8-840e-c4c9a6bc52c1@oracle.com">
<br>
Updated webrev version:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.4/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.4/</a><br>
<br>
<br>
Testing:<br>
One mach5 run with 100 rep counter successfully passed.<br>
To be more confident, I've submitted one more mach5 job
which is in progress. <br>
<br>
Thanks,<br>
Serguei<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>