<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <tt>Vitaly,<br>
      <br>
      Thanks for the question. Reply embedded below.<br>
      <br>
      <br>
    </tt>
    <div class="moz-cite-prefix">On 1/17/13 4:16 PM, Vitaly Davidovich
      wrote:<br>
    </div>
    <blockquote
cite="mid:CAHjP37EiUrQ=_dQpiiew55aPfySMSvpyKuYKpCJQNRaPK3DWuA@mail.gmail.com"
      type="cite">
      <p dir="ltr">Hi Dan,</p>
      <p dir="ltr">Just curious - what's the reason for adding
        OrderAccess::fence() after the pthread_mutex_unlock() calls? Is
        this working around some libc issues or something? I'd expect
        those unlocks to provide the ordering and visibility guarantee.</p>
    </blockquote>
    <br>
    <tt>I presume you are asking about one of these two patterns:<br>
      <br>
      src/os/linux/vm/os_linux.cpp:<br>
      <br>
      4979 void os::PlatformEvent::park() {<br>
      &lt;snip&gt;<br>
      5004&nbsp;&nbsp;&nbsp;&nbsp; _Event = 0 ;<br>
      5005&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; status = pthread_mutex_unlock(_mutex);<br>
      5006&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; assert_status(status == 0, status, "mutex_unlock");<br>
      <font color="blue">5007&nbsp;&nbsp;&nbsp;&nbsp; // Paranoia to ensure our locked and
        lock-free paths interact<br>
        5008&nbsp;&nbsp;&nbsp;&nbsp; // correctly with each other.<br>
        5009&nbsp;&nbsp;&nbsp;&nbsp; OrderAccess::fence();</font><br>
      <br>
      or<br>
      <br>
      5194 void Parker::park(bool isAbsolute, jlong time) {<br>
      &lt;snip&gt;<br>
      5240&nbsp;&nbsp;&nbsp;&nbsp; _counter = 0;<br>
      5241&nbsp;&nbsp;&nbsp;&nbsp; status = pthread_mutex_unlock(_mutex);<br>
      5242&nbsp;&nbsp;&nbsp;&nbsp; assert (status == 0, "invariant") ;<br>
      <font color="blue">5243&nbsp;&nbsp;&nbsp;&nbsp; // Paranoia to ensure our locked and
        lock-free paths interact<br>
        5244&nbsp;&nbsp;&nbsp;&nbsp; // correctly with each other and Java-level accesses. </font><br>
      5245&nbsp;&nbsp;&nbsp;&nbsp; OrderAccess::fence();<br>
      5246&nbsp;&nbsp;&nbsp;&nbsp; return;<br>
      <br>
      <br>
      The short answer is that we use both locked and lock-free paths<br>
      in os::PlatformEvent::park(), os::PlatformEvent::unpark() and in<br>
      Parker::park(). Those optimizations on our part may not play well<br>
      with optimizations done in a looser pthreads implementation. This<br>
      is _not_ a bug in the pthreads implementation. pthreads is allowed<br>
      to presume that everyone is playing nice and using locks; this<br>
      allows pthreads to make some optimizations of their own like<br>
      deferring memory syncing to certain points.<br>
      <br>
      The slightly longer answer is:<br>
      <br>
      These changes were made as part of the fix for the following bug:<br>
      <br>
      &nbsp;&nbsp;&nbsp; 8004902 correctness fixes motivated by contended locking work
      (6607129)<br>
      <br>
      In the last paragraph of the triangular-race.txt attachment of
      8004902,<br>
      I wrote the following about these fence() calls:<br>
      <br>
      <blockquote type="cite">The addition of fence() calls after the
        second and third settings of<br>
        "_counter = 0" was done in the original fix for 6822370 for
        similar<br>
        reasons: worries about weaker memory model semantics. Those two
        settings<br>
        of "_counter = 0" are done while the mutex lock is held and they<br>
        are followed by unlock(_mutex) operations. In all of the
        transaction<br>
        diagrams, I included a "// _counter flushes" comment after T2
        did an<br>
        "unlock(_mutex)" operation. However, there is concern that
        unlock(_mutex)<br>
        may not force _counter to be flushed. It appears that a loose
        pthreads<br>
        implementation is allowed to be lax on memory flushes on unlock
        as long<br>
        as everyone uses locks. Since unpark() always uses locks, I
        think it is<br>
        OK for the transaction diagrams to show T2's unlock() calls
        including the<br>
        "// _counter flushes" comment. In support of this, I'll note
        that the<br>
        original fix for 6822370 does not include fence() calls after
        unpark()<br>
        calls unlock(_mutex). Dave D's latest changes also do not
        include fence()<br>
        calls after unpark() calls unlock(_mutex)</blockquote>
      <br>
      There is some context in the above explanation that assumes that
      you<br>
      have read other parts of triangular-race.txt.<br>
      <br>
      For the extremely long answer about the entire triangular race, I<br>
      refer you to the triangular-race.txt attachment.<br>
      <br>
      Thanks again for the question. Please let me know if I have not
      addressed<br>
      the question to your satisfaction.<br>
      <br>
      Dan<br>
      <br>
      <br>
    </tt>
    <blockquote
cite="mid:CAHjP37EiUrQ=_dQpiiew55aPfySMSvpyKuYKpCJQNRaPK3DWuA@mail.gmail.com"
      type="cite">
      <p dir="ltr"> Thanks</p>
      <p dir="ltr">Sent from my phone</p>
      <div class="gmail_quote">On Jan 17, 2013 1:46 PM, "Daniel D.
        Daugherty" &lt;<a moz-do-not-send="true"
          href="mailto:daniel.daugherty@oracle.com">daniel.daugherty@oracle.com</a>&gt;
        wrote:<br type="attribution">
        <blockquote class="gmail_quote" style="margin:0 0 0
          .8ex;border-left:1px #ccc solid;padding-left:1ex">
          Greetings,<br>
          <br>
          I have been working on the Contended Locking project and there
          are some<br>
          bug fixes that I'd like to push into HSX-25 independently of
          the Contended<br>
          Locking project.<br>
          <br>
          I'm using three different bug IDs to track these very distinct
          bug<br>
          fixes:<br>
          <br>
          &nbsp; &nbsp; 6444286 Possible naked oop related to biased locking
          revocation<br>
          &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; safepoint in jni_exit()<br>
          &nbsp; &nbsp; <a moz-do-not-send="true"
            href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6444286"
            target="_blank">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6444286</a><br>
          &nbsp; &nbsp; <a moz-do-not-send="true"
            href="https://jbs.oracle.com/bugs/browse/JDK-6444286"
            target="_blank">https://jbs.oracle.com/bugs/browse/JDK-6444286</a><br>
          <br>
          &nbsp; &nbsp; 8004902 correctness fixes motivated by contended locking
          work (6607129)<br>
          &nbsp; &nbsp; <a moz-do-not-send="true"
            href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004902"
            target="_blank">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004902</a><br>
          &nbsp; &nbsp; <a moz-do-not-send="true"
            href="https://jbs.oracle.com/bugs/browse/JDK-8004902"
            target="_blank">https://jbs.oracle.com/bugs/browse/JDK-8004902</a><br>
          <br>
          &nbsp; &nbsp; 8004903 VMThread::execute() calls Thread::check_for_valid_safepoint_state()<br>
          &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; on concurrent VM ops<br>
          &nbsp; &nbsp; <a moz-do-not-send="true"
            href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004903"
            target="_blank">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004903</a><br>
          &nbsp; &nbsp; <a moz-do-not-send="true"
            href="https://jbs.oracle.com/bugs/browse/JDK-8004903"
            target="_blank">https://jbs.oracle.com/bugs/browse/JDK-8004903</a><br>
          <br>
          Here is the URL for the webrev:<br>
          <br>
          &nbsp; &nbsp; <a moz-do-not-send="true"
            href="http://cr.openjdk.java.net/%7Edcubed/cl_bug_fix_bucket-webrev/2/"
            target="_blank">http://cr.openjdk.java.net/~dcubed/cl_bug_fix_bucket-webrev/2/</a><br>
          <br>
          These changes have been through two internal rounds of code
          review so I<br>
          think they are ready for wider review. The changes for 8004902
          are very<br>
          tricky and there is long write-up attached to the 8004902 bug
          report that<br>
          explains the intricate details of the "triangular race". The
          changes for<br>
          6444286 and 8004903 are more straight forward.<br>
          <br>
          These changes have been through JPRT build-and-test and have
          also been<br>
          tested with the vm.parallel_class_loading and vm.quick
          subsuites via<br>
          the Aurora ad-hoc testing mechanism.<br>
          <br>
          The current set of reviewers is:<br>
          <br>
          &nbsp; &nbsp; acorn, dholmes, dice<br>
          <br>
          As always, other reviewers are welcome. Comments, questions
          and<br>
          suggestions are also welcome.<br>
          <br>
          Dan<br>
          <br>
        </blockquote>
      </div>
    </blockquote>
    <br>
  </body>
</html>