Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Torvald Riegel <triegel at redhat dot com>]

On Wed, 2015-04-08 at 20:11 +0100, Jonathan Wakely wrote:
> On 08/04/15 18:59 +0200, Torvald Riegel wrote:
> >> +    // set or the maximum number of reader locks is held, then
> >> increment the
> >> +    // reader lock count.
> >> +    // To release decrement the count, then if the write-entered flag
> >> is set
> >> +    // and the count is zero then signal gate2 to wake a queued
> >> writer,
> >> +    // otherwise if the maximum number of reader locks was held
> >> signal gate1
> >> +    // to wake a reader.
> >> +    //
> >> +    // To take a writer lock block on gate1 while the write-entered
> >> flag is
> >> +    // set, then set the write-entered flag to start queueing, then
> >> block on
> >> +    // gate2 while the number of reader locks is non-zero.
> >> +    // To release unset the write-entered flag and signal gate1 to
> >> wake all
> >> +    // blocked readers and writers.
> >
> >Perhaps it would also be useful to have a sentence on how writers and
> >readers are prioritized (e.g., writers preferred if readers already hold
> >the lock, all are equal when writer unlocks, ...).
> 
> How about:
> 
> // This means that when no reader locks are held readers and writers get
> // equal priority. When one or more reader locks is held a writer gets
> // priority and no more reader locks can be taken while the writer is
> // queued.
> 
> >> +    static constexpr unsigned _S_n_readers = ~_S_write_entered;
> >
> >Rename this to _S_max_readers or such?
> 
> Done.
> 
> >>      template<typename _Clock, typename _Duration>
> >>        bool
> >>        try_lock_until(const chrono::time_point<_Clock, _Duration>&
> >> __abs_time)
> >>        {
> >> -       unique_lock<_Mutex> __lk(_M_mut, __abs_time);
> >> -       if (__lk.owns_lock() && _M_state == 0)
> >> +       if (!_M_mut.try_lock_until(__abs_time))
> >> +         return false;
> >
> >I think a non-timed acquisition for the internal lock should be fine.
> >The internal lock is supposed to protect critical sections of finite
> >(and short) length, and the condvars also don't do acquisition with time
> >outs.
> 
> Good point about the condvars re-acquiring the mutex.
> 
> We can get rid of the _Mutex type then, and just use std::mutex, and
> that also means we can provide the timed locking functions even when
> !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK).
> 
> And so maybe we should use this fallback implementation instead of the
> pthread_rwlock_t one when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK), so
> that they have a complete std::shared_timed_mutex (this applies to at
> least Darwin, not sure which other targets).
> 
> Maybe we should also provide a fallback std::timed_mutex based on a
> condvar, I'll put that on the TODO list for stage 1.
> 
> >> +       unique_lock<mutex> __lk(_M_mut, adopt_lock);
> >> +       if (!_M_gate1.wait_until(__lk, __abs_time,
> >> +                                [=]{ return !_M_write_entered(); }))
> >>           {
> >> -           _M_state = _S_write_entered;
> >> -           return true;
> >> +           return false;
> >>           }
> >> -       return false;
> >> +       _M_state |= _S_write_entered;
> >> +       if (!_M_gate2.wait_until(__lk, __abs_time,
> >> +                                [=]{ return _M_readers() == 0; }))
> >> +         {
> >
> >I'd add a comment saying that you have to mimic a full write unlock, so
> >that's why those steps are necessary.
> 
> OK, like this:
> 
> // Wake all threads blocked while the write-entered flag was set.
> 
> >> +           _M_state ^= _S_write_entered;
> >> +           _M_gate1.notify_all();
> >> +           return false;
> >> +         }
> >> +       return true;
> >>        }
> >>  #endif
> >>
> >> @@ -364,7 +399,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >>      unlock()
> >>      {
> >>        {
> >> -       lock_guard<_Mutex> __lk(_M_mut);
> >> +       lock_guard<mutex> __lk(_M_mut);
> >>         _M_state = 0;
> >>        }
> >>        _M_gate1.notify_all();
> >
> >The notification must be *inside* of the critical section.  Otherwise,
> >you're violating the mutex destruction requirements (see 30.4.1.3.1p3):
> >After you set _M_state to 0 and release _M_mut, another thread can go
> >it, acquire, assume it's the last one to own the mutex if that's what
> >the program ensures, and destroy; then, destruction would be concurrent
> >with the call to notify_all.  (Perhaps check other wake-ups / unlocks as
> >well.)
> 
> Thanks. I'll add this comment too:
> 
> // call notify_all() while mutex is held so that another thread can't
> // lock and unlock the mutex then destroy *this before we make the call.
> 
> >>      void
> >>      unlock_shared()
> >>      {
> >> -      lock_guard<_Mutex> __lk(_M_mut);
> >> -      unsigned __num_readers = (_M_state & _M_n_readers) - 1;
> >> -      _M_state &= ~_M_n_readers;
> >> -      _M_state |= __num_readers;
> >> -      if (_M_state & _S_write_entered)
> >> +      lock_guard<mutex> __lk(_M_mut);
> >> +      auto __prev = _M_state--;
> >> +      if (_M_write_entered())
> >>         {
> >> -         if (__num_readers == 0)
> >> +         if (_M_readers() == 0)
> >>             _M_gate2.notify_one();
> >>         }
> >>        else
> >>         {
> >> -         if (__num_readers == _M_n_readers - 1)
> >> +         if (__prev == _S_n_readers)
> >>             _M_gate1.notify_one();
> >>         }
> >
> >I think this needs documentation why we can't miss a wake-up in case
> >_M_write_entered is true and we have a reader overflow.  I think it's
> >not quite obvious why this works.
> >If there is a writer we don't notify gate1 in unlock_shared(), so there
> >is potential to "loose" that information in interleavings with a writer.
> >If the writer acquires the lock eventually, things will work because
> >then, the writer will do the gate1 notification in its unlock path.  If
> >the writer times out, it still does the gate1 notification regardless of
> >the the number of readers registered (because if there is no reader, it
> >will never time out but simply acquire the lock).  Thus, when there is a
> >writer, the writer becomes responsible for gate1 notification, and
> >things work.
> 
> Is this sufficient?
> 
> // No need to notify gate1 because we give priority to the queued
> // writer, and that writer will eventually notify gate1 after it
> // clears the write-entered flag.
> 
> 
> I've attached a diff showing just the changes since the last patch, as
> well as the complete patch against the trunk.
> 

LGTM.  Thanks.