On Sun, Mar 17, 2013 at 2:30 PM, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> wrote:

Le 17/03/13 13:22, Vicente J. Botet Escriba a écrit :

I have started today.

I'm not confident with this refactoring

         void unlock_shared()
         {
             boost::unique_lock<boost::mutex> lk(state_change);
-            bool const last_reader=!--state.shared_count;
 
-            if(last_reader)
-            {
-                if(state.upgrade)
-                {
-                    state.upgrade=false;
-                    state.exclusive=true;
-                    upgrade_cond.notify_one();
-                }
-                else
-                {
-                    state.exclusive_waiting_blocked=false;
-                }
+            bool const last_reader = !state.unlock_shared();
+
+            if (last_reader) {
+                upgrade_cond.notify_one();

where

            unsigned unlock_shared () {
                bool const last_reader = !--shared_count;
           
                if (last_reader) {
                    if (upgrade) {
                        upgrade=false;
                        exclusive=true;
                    } else {
                        exclusive_waiting_blocked=false;
                    }
                }
           
                return shared_count;
            }

In the current code
                upgrade_cond.notify_one();

is done if !--state.shared_count and state.upgrade

After the pacth
                upgrade_cond.notify_one();
 
is done if !--state.shared_count independently of the state of state.upgrade.

I guess that unlock_shared () should return whether the mutex is downgraded

            bool unlock_shared () {
                bool const last_reader = !--shared_count;
           
                if (last_reader) {
                    if (upgrade) {
                        upgrade=false;
                        exclusive=true;
                        return true;
                    } else {
                        exclusive_waiting_blocked=false;
                    }
                }
                return false;
            }

and the code is replaced by

+            bool const downgraded = state.unlock_shared();
+
+            if (downgraded) {
+                upgrade_cond.notify_one();

It is a little bit worse, the solution should be something like

        void unlock_shared()
        {
            boost::unique_lock<boost::mutex> lk(state_change);

            state.unlock_shared();
            if (state.is_last_shared())
            {
              if (state.unlock_shared_downgrades())
              {
                upgrade_cond.notify_one();
              }
              release_waiters();
            }
        }

where

            bool state::is_last_shared () const
            {
                return !shared_count ;
            }
            void unlock_shared ()
            {
                --shared_count;
            }
            bool unlock_shared_downgrades()

            {
                  if (upgrade) {
                      upgrade=false;
                      exclusive=true;
                      return true;
                  } else {
                      exclusive_waiting_blocked=false;
                      return false;
                  }
            }

You are correct in that my patch introduces upgrade_cond.notify_one in unlock_shared also if state.upgrade is not set. My impression, however, was that this additional notification is merely an simplification "inefficiency" that should not impact the correctness of the implementation. Upgrade_cond has a fairly simple usage pattern that only involves unlock_shared and unlock_upgrade_and_lock, and I could not see any side-effects of the simplification.

 

For what I have seen the management of exclusive_waiting_blocked is not hidden by your patch.

It's partly hidden. Usage of exclusive_waiting_blocked is fully hidden, but the flag still have to be explicitly set in a few places.

 

Another point: the following refactoring sets

  state.upgrade=false;

before been sure tat can move to lock.

        void unlock_upgrade_and_lock()
        {
#if defined BOOST_THREAD_PROVIDES_INTERRUPTIONS
            boost::this_thread::disable_interruption do_not_disturb;
#endif

             boost::unique_lock<boost::mutex> lk(state_change);

-            --state.shared_count;
-            while(state.shared_count)
-            {
+
+            state.unlock_upgrade();
+            while (!state.can_lock())
                 upgrade_cond.wait(lk);

-            }
-            state.upgrade=false;
-            state.exclusive=true;

+           
+            state.lock();

In this case another thread could take the mutex with lock_upgrade which should not be allowed.

Good point Vicente. This is indeed a weakness with my patch that I did not take into consideration.

Thank you very much for your thorough review Vicente. I think I'll need investigate options for how to address your comments, and come back to you.