|
|
Threads-Devel : |
Subject: Re: [Threads-devel] RFC: 1st pthread shared_mutex refactoring patch
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2013-03-17 19:03:52
Le 17/03/13 23:21, Fredrik Orderud a écrit :
> On Sun, Mar 17, 2013 at 2:30 PM, Vicente J. Botet Escriba
> <vicente.botet_at_[hidden] <mailto: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.
>
While applying you patch (not done completely yet) I introduced some
assertions that show that the implementation (mine) is not correct (see
attached patch). When you uncomment the assertions the regression tests
t_shared don't asserts.
I suspect that the error is not on the patch but on the trunk code. I
will redo the same assertions on trunk without any refactoring to check it.
BTW if you get to catch where the bugs are, even if you don't have a
solution, please signal it to me as soon as possible, this will help me
a lot.
Best,
Vicente