<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <br>
    Hi Bill,<br>
    <br>
    <div class="moz-cite-prefix">On 2015-06-24 16:58, bill pittore
      wrote:<br>
    </div>
    <blockquote cite="mid:558AC57C.7050408@oracle.com" type="cite">
      <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
      Hi Bengt,<br>
       This seems to eliminate the TSO issues rather cleanly. </blockquote>
    <br>
    Thanks for looking at this!<br>
    <br>
    <blockquote cite="mid:558AC57C.7050408@oracle.com" type="cite">One
      small nit is that the comments in concurrentMarkThread.hpp and
      CollectedHeap.cpp talk about setting/clearing 'flags' whereas now
      it's setting a state variable.<br>
    </blockquote>
    <br>
    Agreed. I've updated the comment this way:<br>
    <br>
    diff --git a/src/share/vm/gc/g1/concurrentMarkThread.hpp
    b/src/share/vm/gc/g1/concurrentMarkThread.hpp<br>
    --- a/src/share/vm/gc/g1/concurrentMarkThread.hpp<br>
    +++ b/src/share/vm/gc/g1/concurrentMarkThread.hpp<br>
    @@ -81,11 +81,11 @@<br>
       void set_in_progress()   { assert(_state == Started, "must be
    starting a cycle"); _state = InProgress; }<br>
       bool in_progress()       { return _state == InProgress; }<br>
     <br>
    -  // This flag returns true from the moment a marking cycle is<br>
    +  // Returns true from the moment a marking cycle is<br>
       // initiated (during the initial-mark pause when started() is
    set)<br>
       // to the moment when the cycle completes (just after the next<br>
       // marking bitmap has been cleared and in_progress() is<br>
    -  // cleared). While this flag is true we will not start another
    cycle<br>
    +  // cleared). While during_cycle() is true we will not start
    another cycle<br>
       // so that cycles do not overlap. We cannot use just
    in_progress()<br>
       // as the CM thread might take some time to wake up before
    noticing<br>
       // that started() is set and set in_progress().<br>
    <br>
    <br>
    I'll push my change with this this updated comment.<br>
    <br>
    Thanks again for reviewing this!<br>
    <br>
    Bengt<br>
    <br>
    <br>
    <blockquote cite="mid:558AC57C.7050408@oracle.com" type="cite"> <br>
      bill<br>
      <br>
      <div class="moz-cite-prefix">On 6/24/2015 3:28 AM, Bengt Rutisson
        wrote:<br>
      </div>
      <blockquote cite="mid:558A5C2D.3020504@oracle.com" type="cite">
        <meta content="text/html; charset=utf-8"
          http-equiv="Content-Type">
        <br>
        <br>
        Hi Vitaly,<br>
        <br>
        <div class="moz-cite-prefix">On 2015-06-23 23:53, Vitaly
          Davidovich wrote:<br>
        </div>
        <blockquote
cite="mid:CAHjP37EEE=HZbgRvaTaCU=UueFqcKWOwEP0xBZBENHqZRmtyZQ@mail.gmail.com"
          type="cite">
          <p dir="ltr">Naive question - could this be converted into a
            numeric state field that indicates the lifecycle (e.g 1 = in
            progress, 2 = started)?</p>
        </blockquote>
        <br>
        Good question! That's a much simpler and more stable solution.<br>
        <br>
        New webrev:<br>
        <a moz-do-not-send="true" class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Ebrutisso/8129626/webrev.02/">http://cr.openjdk.java.net/~brutisso/8129626/webrev.02/</a><br>
        <br>
        Thanks,<br>
        Bengt<br>
        <br>
        <br>
        <br>
        <blockquote
cite="mid:CAHjP37EEE=HZbgRvaTaCU=UueFqcKWOwEP0xBZBENHqZRmtyZQ@mail.gmail.com"
          type="cite">
          <p dir="ltr">sent from my phone</p>
          <div class="gmail_quote">On Jun 23, 2015 5:42 PM, "bill
            pittore" <<a moz-do-not-send="true"
              href="mailto:bill.pittore@oracle.com">bill.pittore@oracle.com</a>>


            wrote:<br type="attribution">
            <blockquote class="gmail_quote" style="margin:0 0 0
              .8ex;border-left:1px #ccc solid;padding-left:1ex">
              <div bgcolor="#FFFFFF" text="#000000"> Generally when you
                have a storestore on the write side you need a loadload
                on the read side to prevent the second read from
                floating above the first one. The reading thread could
                read in_progress as 0 before it reads starting.
                Meanwhile the write thread writes 1 to in_progress,
                issues storestore, clears starting. Reading thread then
                reads starting as 0. I don't know if the CGC mutex
                somehow eliminates this issue as I'm not familiar with
                the code in detail.<br>
                <br>
                bill<br>
                <br>
                <div>On 6/23/2015 4:25 PM, Bengt Rutisson wrote:<br>
                </div>
                <blockquote type="cite"> <br>
                  Hi everyone,<br>
                  <br>
                  Could I have a couple of reviews for this change?<br>
                  <br>
                  <a moz-do-not-send="true"
                    href="https://bugs.openjdk.java.net/browse/JDK-8129626"
                    target="_blank">https://bugs.openjdk.java.net/browse/JDK-8129626</a><br>
                  <a moz-do-not-send="true"
                    href="http://cr.openjdk.java.net/%7Ebrutisso/8129626/webrev.00/"
                    target="_blank">http://cr.openjdk.java.net/~brutisso/8129626/webrev.00/</a><br>
                  <br>
                  We need to add a barrier between the calls to
                  set_in_progress() and clear_started() to make sure
                  that other threads sees the correct value when they
                  use  ConcurrentMarkThread::during_cycle().<br>
                  <br>
                  Thanks to Per and Bertrand for helping out identifying
                  and sorting this out.<br>
                  <br>
                  From the bug report:<br>
                  <br>
                  ConcurrentMarkThread::during_cycle() is implemented
                  as: <br>
                  <br>
                  bool during_cycle() { return started() ||
                  in_progress(); } <br>
                  <br>
                  So, it checks both ConcurrentMarkThread::_started and
                  ConcurrentMarkThread::_in_progress and they are meant
                  to overlap. That is, we should not set _started to
                  false until after we have set _in_progress to true.
                  This is done in sleepBeforeNextCycle(): <br>
                  <br>
                  void ConcurrentMarkThread::sleepBeforeNextCycle() { <br>
                    // We join here because we don't want to do the
                  "shouldConcurrentMark()" <br>
                    // below while the world is otherwise stopped. <br>
                    assert(!in_progress(), "should have been cleared");
                  <br>
                  <br>
                    MutexLockerEx x(CGC_lock,
                  Mutex::_no_safepoint_check_flag); <br>
                    while (!started() && !_should_terminate) { <br>
                      CGC_lock->wait(Mutex::_no_safepoint_check_flag);

                  <br>
                    } <br>
                  <br>
                    if (started()) { <br>
                      set_in_progress(); <br>
                      clear_started(); <br>
                    } <br>
                  } <br>
                  <br>
                  On non-TSO platforms there is a risk that the write to
                  _in_progress (from set_in_progress()) is seen by other
                  threads after the write to _started (in
                  clear_started()). In that case there is a window when
                  during_cycle() may return false even though we are in
                  a concurrent cycle. <br>
                  <br>
                  <br>
                  Thanks,<br>
                  Bengt<br>
                </blockquote>
                <br>
              </div>
            </blockquote>
          </div>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>