Hi Chris,
Please, let me know if you have more thoughts on this or it is
okay to push as it is.
And thank you a lot for reviewing this!
Thanks,
Serguei
On 10/8/19 13:20, serguei.spit...@oracle.com wrote:
Hi
Chris,
On 10/8/19 12:56 PM, Chris Plummer
wrote:
Hi Serguei,
The just seems like an odd coding pattern to me:
// Block until the suspender thread competes the tested
threads suspension
agent_lock(jni);
agent_unlock(jni);
I do not consider it as odd but normal.
We have similar coding patterns in the jdwp agent.
The agent_unlock(jni) call can be moved to the end of the function
if you like.
It does not matter in this particular case but will look "better".
Wouldn't you normally use
wait/notify in this situation? Some like:
agent_lock(jni);
if (!suspenderDone) {
agent_wait(jni);
}
agent_unlock(jni);
The above will make synchronization more complex.
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?).
Yes, it is the case.
Thanks,
Serguei
Ping...
Thanks,
Serguei
Hi Alex, Chris and David,
The mach5 testing in 100 runs loop on all platform
discovered a race in new test.
It is between the native suspendTestedThreads() called on
the suspender thread
and the checkSuspendedStatus() calling native
checkTestedThreadsSuspended().
It occurred that the iteration count and sleep time in
checkSuspendedStatus() can be not enough:
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 }
So, I've decided to refactor/update the test a little bit:
1. Now the checkSuspendedStatus() is:
100 private boolean checkSuspendedStatus() throws RuntimeException {
101 log("Main: checking all tested threads have been suspended");
102 return checkTestedThreadsSuspended();
103 }
2. A raw monitor agent_monitor is added to the native
agent.
It is created and locked in the
Java_ThreadToSuspend_init() function called at
the beginning of the ThreadToSuspend.run() execution of
the suspender thread
and unlocked at the end of
Java_ThreadToSuspend_suspendTestedThreads().
The
Java_SuspendWithCurrentThread_checkTestedThreadsSuspended()
grabs the agent_monitor
on the Main thread to wait until the
suspendTestedThreads() completes its work.
3. Also, the results[] array is always created and used
locally in the functions
where it is needed: suspendTestedThreads() and
resumeTestedThreads().
Forgot to tell about one more change:
4. The releaseTestedThreads() is renamed to
releaseTestedThreadsInfo() and moved
to the end of the Main thread run() method.
Also wanted to thank Alex for sharing good suggestions on
the sync approaches.
Thanks,
Serguei
Updated webrev version:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.4/
Testing:
One mach5 run with 100 rep counter successfully passed.
To be more confident, I've submitted one more mach5 job
which is in progress.
Thanks,
Serguei
|
