|
|
Threads-Devel : |
Subject: Re: [Threads-devel] RFC: 1st pthread shared_mutex refactoring patch
From: Fredrik Orderud (forderud_at_[hidden])
Date: 2013-03-17 18:21:23
On Sun, Mar 17, 2013 at 2:30 PM, Vicente J. Botet Escriba <
vicente.botet_at_[hidden]> 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.
Regards,
Fredrik