Thanks, Chris!
Serguei


On 10/9/19 10:14, Chris Plummer wrote:
Hi Serguei,

It's ok to push.

thanks,

Chris

On 10/8/19 10:47 PM, serguei.spit...@oracle.com wrote:
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


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








Reply via email to