[cpp-threads] Re: Thread API interface tweaks

[Boehm, Hans]

I agree with Peter.  Reading _id while another thread is changing it,
even if it is between irrelevant values, is a data race, and hence
invokes undefined behavior.  And that makes sense: consider the case in
which a thread_id is bigger than something I can read and write
atomically, or might not be sufficiently aligned for atomicity.  I might
read two inconsistent half thread_ids, the concatenation of which might
be my id, causing very bad things to happen.

The code here really is broken under some slightly uncommon conditions;
we're not just dealing in technicalities.   You do need atomics.

I think this is actually one of the relatively rare cases in which no
ordering is required, and a <raw> atomic access to id_ makes sense.
Either the next operation acquires a lock, or it accesses data that's
temporarily private to me.

I don't see why count has to be atomic or needs release semantics.  I
think we can arrange to only access it by the lock holder.

Hans

> -----Original Message-----
> From: cpp-threads-bounces at decadentplace.org.uk 
> [mailto:cpp-threads-bounces at decadentplace.org.uk] On Behalf 
> Of Peter Dimov
> Sent: Wednesday, August 30, 2006 3:43 PM
> To: Howard Hinnant
> Cc: C++ threads standardisation; David Miller; Alisdair 
> Meredith; Terrence.Miller at Sun.COM; David Miller; Bill 
> Seymour; P.J. Plauger (Dinkumware Ltd); Bjarne Stroustrup; 
> Bronis R. de Supinski; Eric Niebler; Pete Becker
> Subject: Re: [cpp-threads] Re: Thread API interface tweaks
> 
> Howard Hinnant wrote:
> > I appreciate the free analysis. :-)  Here's my understanding:
> >
> > On Aug 30, 2006, at 4:20 PM, Peter Dimov wrote:
> >
> >>> template <class Mutex>
> >>> class recursive_mutex
> >>> {
> >>> private:
> >>>     Mutex& mut_;
> >>>     unsigned count_;
> >>>     thread_id id_;
> >>> public:
> >>>     explicit recursive_mutex(Mutex& mut)
> >>>         : mut_(mut), count_(0) {}  // id_ has "not a thread" value
> >>>
> >>>     void lock()
> >>>     {
> >>>         thread_id self = thread_id::current();
> >>>         if (id_ == self)
> >>
> >> Here...
> >
> > If id_ != self, then obviously this branch isn't taken.  
> Also no other 
> > thread is capable of setting id_ to self.  Some other thread might 
> > change id_, but only from one value that is not self to 
> another value 
> > that is not self.  If id_ == self, then no other thread is 
> capable of 
> > changing the value of id_ at all.  And in this case, no 
> other thread 
> > has the ability to change count_ at all.
> 
> I'm not saying that there are correctness problems with the 
> test. My point is that
> 
>     if( id_ == self )
> 
> reads id_, and ...
> 
> >>>             ++count_;
> >>>         else
> >>>         {
> >>>             mut_.lock();
> >>>             id_ = self;
> 
> ... these ...
> 
> >>>             store_release(&count_, 1);
> >>>         }
> >>>     }
> >>>
> >>>     void unlock()
> >>>     {
> >>>         if (--count_ == 0)
> >>>         {
> >>>             id_ = thread_id();  // id_ = "not a thread"
> 
> ... two statements write to id_. It seems to me that there is 
> a legitimate possibility for the read to overlap one of the 
> writes. Therefore, the type of id_ must be atomic. I think. 
> 
> 
> --
> cpp-threads mailing list
> cpp-threads at decadentplace.org.uk
> http://www.decadentplace.org.uk/cgi-bin/mailman/listinfo/cpp-threads
>