From: Torvald Riegel <triegel@redhat.com>
To: Jonathan Wakely <jwakely@redhat.com>
Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [patch] Fix shared_timed_mutex::try_lock_until() et al
Date: Fri, 10 Apr 2015 00:11:00 -0000 [thread overview]
Message-ID: <1428624680.924.29.camel@triegel.csb> (raw)
In-Reply-To: <20150408191159.GQ9755@redhat.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.
next prev parent reply other threads:[~2015-04-10 0:11 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-07 14:28 Jonathan Wakely
2015-04-08 16:59 ` Torvald Riegel
2015-04-08 19:12 ` Jonathan Wakely
2015-04-08 19:38 ` Jonathan Wakely
2015-04-10 0:16 ` Torvald Riegel
2015-04-10 8:38 ` Jonathan Wakely
2015-04-10 9:56 ` Torvald Riegel
2015-04-10 10:21 ` Jonathan Wakely
2015-04-10 0:11 ` Torvald Riegel [this message]
2015-04-11 11:48 ` Jonathan Wakely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1428624680.924.29.camel@triegel.csb \
--to=triegel@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=libstdc++@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).