Hi Chris and Alex,
I discussed this with Alex before preparing and sending this webrev.
We considered to use a mutex but then I decided that using a raw monitor
is better.
Thanks,
Serguei
On 10/8/19 18:21, Chris Plummer wrote:
It's not really being used as a mutex. It's being used as completion
flag to ensure proper ordering. That's why I thought it odd.
Chris
On 10/8/19 6:07 PM, Alex Menkov wrote:
Hi Chris,
Actually this "lock" is used as a mutex.
So agent_lock acquires the mutex, agent_unlock releases the mutex.
Serguei,
The fix looks good to me.
Some cosmetic notes (updated webrev is not required):
SuspendWithCurrentThread.java
52 public static void main(String args[]) throws Exception {
please make it java-style: "String[] args"
79 ThreadToSuspend[]threads = new ThreadToSuspend[threadsCount];
missed space: ThreadToSuspend[] threads
--alex
On 10/08/2019 12:56, 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);
Wouldn't you normally use wait/notify in this situation? Some like:
agent_lock(jni);
if (!suspenderDone) {
agent_wait(jni);
}
agent_unlock(jni);
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?).
thanks,
Chris
On 10/8/19 11:50 AM, serguei.spit...@oracle.com wrote:
Ping...
Thanks,
Serguei
On 10/7/19 5:25 PM, serguei.spit...@oracle.com wrote:
On 10/7/19 17:14, serguei.spit...@oracle.com wrote:
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
