[boost] Re: Lock-free shared_ptr on g++/x86, test/review needed

From: Alexander Terekhov (terekhov_at_[hidden])
Date: 2005-04-05 06:36:39

Peter Dimov wrote:
[...]
> Yes, but in this case threads are not a problem (we're in release,
> destroying the last remaining shared_ptr; if we read weak_count_ == 1, no
> other thread can be creating/destroying weak pointers because of basic
> guarantee). Msync is not a problem because we've just acquired in the
> decrement.

Yeah. Hello Peter, remember original

void release() throw() {
  if (!use_count_.decrement()) { // "full" release-acquire protocol
    dispose();
    if (!self_count_.decrement(msync::rel, count::may_not_store_min))
      destruct();
  }
}

?

(msync::rel on self_count_.decrement() was meant for "result is not 
min" case only; with naked op for "old value == min + 1" case.) 

Sure, it can be safely transformed to 

void release() throw() {
  if (!use_count_.decrement()) { // "full" release-acquire protocol
    dispose();
    if (self_count_.load(msync::naked_competing) == 1 || // min == 0
        !self_count_.decrement(msync::rel))
      destruct();
  }
}

but ...

> 
> Now that I think of it, compiler value propagation isn't a problem either
> because of the "memory" clobber on the decrement. So the cast to volatile
> can be removed.
> 
>     void release() // nothrow
>     {
>         if( atomic_exchange_and_add( &use_count_, -1 ) == 1 )
>         {
>             dispose();
> 
>             if( weak_count_ == 1 ) // no weak ptrs

(this doesn't convey the notion of (potentially) competing accesses; who 
says that implementation can't read it bit-after-bit-and-sum-it, bytes 
aside for a moment... just for the sake of breaking your logic?)

>             {
>                 destroy();
>             }
>             else
>             {
>                 weak_release();
>             }
>         }
>     }

... you also have and manage client-provided "deleter" objects and 
I see no reason why weak_ptr clients may NOT have access to them 
(resulting in similar problem with respect to access and 
destruction as with Whitehead's mutex). 

So to be safe, you need

void release() throw() {
  if (!use_count_.decrement()) { // "full" release-acquire protocol
    dispose();
    if (self_count_.load(msync::ccacq) == 1 || // min == 0
        !self_count_.decrement()) // "full" release-acquire protocol
      destruct();
  }
  // "full" means either "value dependent" or fully-fenced operation
}

So again, 

http://gcc.gnu.org/ml/libstdc++/2005-04/msg00000.html

I suggest that you hide that access in asm (and consider using the 
same operation in weak_release() too... for the sake of KISS-on-IA32 
and "easy" porting to RISC archs, where "count::may_not_store_min" 
is surely better in both release() and weak_release()).

regards,
alexander.