[boost] Re: Boost.Threads: Do we need all those mutexes?

From: Michael Glassford (glassfordm_at_[hidden])
Date: 2004-07-05 13:08:59

Anthony Williams wrote:

> Michael Glassford <glassfordm_at_[hidden]> writes:
> 
> 
>>Anthony Williams wrote:
> 
> 
>>>Michael Glassford <glassfordm_at_[hidden]> writes:
> 
> 
>>>>Anthony Williams wrote:
> 
> 
>>>>>Once you have a write lock (the "higher class" of lock), then you keep your
>>>>>write lock until you release it. If you want a read lock for the whole
>>>>>time, but only a write lock for a portion of that time, then you write code
>>>>>like the first example. Your "reverse case" actually fulfils that
>>>>>requirement, since there is a portion of the code that only needs a read
>>>>>lock. Therefore it should look like:
>>>>>
>>>>>      void f(read_write_mutex m)
>>>>>      {
>>>>>          read_write_mutex::read_lock r(m); // we need at least a read lock
>>>>>                                            // for everything
>>>>>          {
>>>>>              read_write_mutex::write_lock w(r);
>>>>>              // do stuff that needs the write lock here
>>>>>          }
>>>>>          if (...)
>>>>>          {
>>>>>              // we only need the read lock here
>>>>>              //...
>>>>>          }
>>>>>          {
>>>>>              read_write_mutex::write_lock w(r);
>>>>>              // do more stuff that needs the write lock here
>>>>>          }
>>>>>      }
>>>>
>>>>Again, this might be a better way to write it, but how do you prevent
>>>>someone from writing it the first way?
>>>
>>>See above.
>>
>>The above code has a problem, however. This:
>>
>>     void f(read_write_mutex m)
>>     {
>>         read_write_mutex::read_lock w(m);
>>             //Block until we get read-lock
>>
>>         //do stuff
>>
>>         read_write_mutex::write_lock(r);
>>             //Convert to write-lock; may fail; then what?
> 
> 
> Don't allow it to fail --- block.

That's a bad idea because of deadlock issues, as I (and you) mention 
below. And in any case, the two pieces of code are still not the same. 
The first will often block or deadlock at the promotion, the second will 
never block at the demotion.

> 
>>         //do more stuff
>>   }
>>
>>Is not the same as this:
>>
>>     void f(read_write_mutex m)
>>     {
>>         read_write_mutex::read_write_lock l(m, write_locked);
>>            //Block until we get write-lock
>>
>>         //do stuff
>>
>>         l.demote();
>>             //Convert to read-lock; never fails
>>
>>         //do more stuff
>>   }
>>
>>The reason is that the first example always succeeds in getting the write
>>lock eventually, and lock demotion always succeeds as well, so we always get
>>the lock we want. In the second example, we can always get the read-lock,
>>but the lock promotion may fail, so we may never get the write lock.
> 
> 
> Not an issue if promotion is blocking.
> 
> 
>>The reason lock promotion may fail is that if two threads holding read locks
>>are both waiting for promotion to write-locks, neither will ever get the
>>write lock (which can't be obtained until all other read-locks have been
>>released) and they will deadlock. So, unless you know of a better solution,
>>at most one thread must ever be allowed to wait for lock promotion and any
>>other promotion attempts must fail.
> 
> 
> In which case, don't do it like that.I see this as the fundamental problem of
> promotion 

I agree.

Which is why I recommended getting the write lock first and later 
demoting it, as opposed to the solution you originally suggested which 
requires getting the read-lock first and then promoting it.

 > if the idea of promotion is to ensure that no one has modified
> the data since you read it, then if two threads have read locks and try and
> promote them to write locks, either you deadlock or one will fail. If it
> fails, what do you do? You have to release your read lock to let the other
> thread do the write, and then try again. 

There are times when even this could be useful for efficiency, I 
suppose--you try to promote first, then if it fails you release the 
read-lock, get a write-lock, redo (as little as possible of) the work 
you did when you had the read-lock, then do the writing. However...

In my mind demotion is more important: you make modifications, then 
demote to a read-lock to allow other readers access to the resource 
while you continue to read from it yourself, with the guarantee that the 
changes you just made have not been modified by another thread before 
you use them.

 > This makes me think that such a
> feature is not particularly useful in the general case. Maybe a try_write_lock
> variant is in order?

Are you referring to one that implements something like try_promote()? 
If so, I agree to the point that I have so far provided only 
try_promote() and timed_promote() functions in Boost.Threads, but no 
promote() function at all due to the problems we both raised above.

> I might code it so that the write_lock does the release/reacquire itself, but
> this is a difficult issue. 

I have an experimental set_lock member in the read/write locks that does 
this (release/reacquire): it changes from one lock state to another, 
using the best option possible. For example, when changing from a 
read-lock to a write-lock, it first uses try_promote() (if available); 
if that fails, it unlocks then write-locks. I thought it also returned a 
value that indicated whether it had to release the lock first, but see 
that it doesn't; maybe I need to change that.

 > Allowing deadlocks is not a good thing, but I
> really don't like the idea of the try_xxx locks because it raises the issue of
> "have I got the lock or not?". I'm also not a fan of control by member
> functions --- this is a resource, so we should use RAII.

It is using RAII, at least as much as is commonly done. It's pretty 
common to acquire the resource in the constructor, change its state with 
a member function, and release it (if applicable) in the destructor. The 
lock() and unlock() methods of the Boost.Threads lock classes are 
another example of the same thing, as are smart pointer assignment 
operators, etc. It would be pretty limiting to remove all these.

>>>>I think one of the most common use cases for lock demotion would be: obtain
>>>>a write lock and make modifications to a resource; demote to a read lock to
>>>>release other readers and use the resource, making sure that it is still in
>>>>the state it was when we had the write lock (i.e., that no other thread
>>>>obtained a write lock and changed it between the time we released our write
>>>>lock and the time we obtained our read lock).
>>>>
>>>>With a read/write lock, this is expressed in a straightforward way:
>>>>
>>>>    void f(read_write_mutex m)
>>>>    {
>>>>        read_write_mutex::write_lock l(m);
>>>>
>>>>        //Modify the resource
>>>>
>>>>        l.demote(); //Release other readers
>>>>
>>>>        //Use the resource
>>>>    }
>>>>
>>>>With the separate read-lock and write-lock, how do you express this in a way
>>>>that is straightforward, doesn't do unnecessary work when the locks are
>>>>released, etc.?
>>>
>>>      void f(read_write_mutex m)
>>>      {
>>>          read_write_mutex::read_lock r(m);
>>>          {
>>>              read_write_mutex::write_lock l(r);
>>>              //Modify the resource
>>>          } // release other readers
>>>          //Use the resource
>>>      }
>>
>>This has the same problem I mention above. Unless I'm missing something, it
>>also performs unnecessary work (a promotion followed by a demotion, as
>>opposed to only a demotion in my example).
> 
> 
> I agree there is the possibility for unnecessary work, but in my view it
> clearly marks the region where writing is done. With a read-write lock, I can
> write:
> 
> void g(read_write_mutex::read_write_lock& l);
> 
> void f(read_write_mutex& m)
> {
>     read_write_mutex::read_write_lock l(m,write_locked);
>     g(l);
>     // Are we still locked for writing, or just reading?
> }
> 
> void f2(read_write_mutex& m)
> {
>     read_write_mutex::read_write_lock l(m,read_locked);
>     g(l);
>     // Are we still locked for just reading, or for writing as well?

True. The same is true of the unlock(() and lock() members of lock 
classes, the assignment operator of smart pointers, and, in general, of 
any function that can modify its arguments. What if you write:

    void g(const read_write_mutex::read_write_lock& l);

instead?

> }
> 
> Whereas with separate locks it's clear what's happening when I write:
> 
> void g(read_write_mutex::read_lock& l);
> void g(read_write_mutex::write_lock& l);
> 
> void f(read_write_mutex& m)
> {
>     read_write_mutex::read_lock l(m);
>     g(l);
>     // We're still locked for just reading, whether or not g did writing
> }
> 
> void f2(read_write_mutex& m)
> {
>     read_write_mutex::write_lock l(m);
>     g(l);
>     // We're still locked for writing
> }

I agree completely that separate locks are a good idea in most cases, 
but don't think that they work very well with lock promotion or 
demotion. What I'm most inclined to do at the moment is to have separate 
read_lock and write_lock classes that don't support promotion or 
demotion, and a combined read_write_lock class that supports both for 
those cases where promotion and demotion are required.

Mike