[TSan] Fixed data race: added compiler barriers, tuned memory ordering

diff --git a/cds/gc/details/hp.h b/cds/gc/details/hp.h
index cfd68b9348a6b2f9101faf09283483617a43e765..84c7702051d2fa982e919cc4e5a7c12fc12534f0 100644 (file)
--- a/cds/gc/details/hp.h
+++ b/cds/gc/details/hp.h
@@ -112,7 +112,7 @@ namespace cds {
                 typedef retired_vector_impl::iterator  iterator;
 
                 /// Constructor
-                retired_vector( const cds::gc::hp::GarbageCollector& HzpMgr ); // inline
+                explicit retired_vector( const cds::gc::hp::GarbageCollector& HzpMgr ); // inline
                 ~retired_vector()
                 {}
 
@@ -185,7 +185,7 @@ namespace cds {
                 char padding2[cds::c_nCacheLineSize];
 
                 /// Ctor
-                hp_record( const cds::gc::hp::GarbageCollector& HzpMgr ); // inline
+                explicit hp_record( const cds::gc::hp::GarbageCollector& HzpMgr ); // inline
                 ~hp_record()
                 {}
 
@@ -294,16 +294,23 @@ namespace cds {
             {
                 hplist_node *                    m_pNextNode; ///< next hazard ptr record in list
                 atomics::atomic<OS::ThreadId>    m_idOwner;   ///< Owner thread id; 0 - the record is free (not owned)
-                atomics::atomic<bool>            m_bFree;     ///< true if record if free (not owned)
+                atomics::atomic<bool>            m_bFree;     ///< true if record is free (not owned)
 
                 //@cond
-                hplist_node( const GarbageCollector& HzpMgr )
+                explicit hplist_node( const GarbageCollector& HzpMgr )
                     : hp_record( HzpMgr ),
                     m_pNextNode( nullptr ),
                     m_idOwner( OS::c_NullThreadId ),
                     m_bFree( true )
                 {}
 
+                hplist_node( const GarbageCollector& HzpMgr, OS::ThreadId owner )
+                    : hp_record( HzpMgr ),
+                    m_pNextNode( nullptr ),
+                    m_idOwner( owner ),
+                    m_bFree( false )
+                {}
+
                 ~hplist_node()
                 {
                     assert( m_idOwner.load( atomics::memory_order_relaxed ) == OS::c_NullThreadId );
@@ -338,7 +345,7 @@ namespace cds {
             ~GarbageCollector();
 
             /// Allocate new HP record
-            hplist_node * NewHPRec();
+            hplist_node * NewHPRec( OS::ThreadId owner );
 
             /// Permanently deletes HPrecord \p pNode
             /**

diff --git a/cds/intrusive/details/split_list_base.h b/cds/intrusive/details/split_list_base.h
index f6b410846d6ee759e796c75115e377536397b494..d54f26462f0d0fd22b8d5a9b4fde5131099016c0 100644 (file)
--- a/cds/intrusive/details/split_list_base.h
+++ b/cds/intrusive/details/split_list_base.h
@@ -649,8 +649,9 @@ namespace cds { namespace intrusive {
             /// Allocates auxiliary node; can return \p nullptr if the table exhausted
             aux_node_type* alloc_aux_node()
             {
+                aux_node_segment* aux_segment = m_auxNodeList.load( memory_model::memory_order_acquire );
+
                 for ( ;; ) {
-                    aux_node_segment* aux_segment = m_auxNodeList.load( memory_model::memory_order_relaxed );
                     assert( aux_segment != nullptr );
 
                     // try to allocate from current aux segment
@@ -671,7 +672,9 @@ namespace cds { namespace intrusive {
                     aux_node_segment* new_aux_segment = allocate_aux_segment();
                     new_aux_segment->next_segment = aux_segment;
                     new_aux_segment->aux_node_count.fetch_add( 1, memory_model::memory_order_relaxed );
-                    if ( m_auxNodeList.compare_exchange_strong( aux_segment, new_aux_segment, memory_model::memory_order_relaxed, atomics::memory_order_relaxed ))
+                    CDS_COMPILER_RW_BARRIER;
+
+                    if ( m_auxNodeList.compare_exchange_strong( aux_segment, new_aux_segment, memory_model::memory_order_release, atomics::memory_order_acquire ))
                         return new( new_aux_segment->segment()) aux_node_type();
 
                     free_aux_segment( new_aux_segment );

diff --git a/cds/urcu/details/base.h b/cds/urcu/details/base.h
index 5a2abcc09fc4fc599f832dd380f3fcc24ee23ce5..84bf8fb85a205449775f35a5d237443eca2de05d 100644 (file)
--- a/cds/urcu/details/base.h
+++ b/cds/urcu/details/base.h
@@ -328,6 +328,11 @@ namespace cds {
                     , m_idOwner( cds::OS::c_NullThreadId )
                 {}
 
+                explicit thread_list_record( OS::ThreadId owner )
+                    : m_pNext( nullptr )
+                    , m_idOwner( owner )
+                {}
+
                 ~thread_list_record()
                 {}
             };
@@ -369,12 +374,14 @@ namespace cds {
 
                     // No records available for reuse
                     // Allocate and push a new record
-                    pRec = allocator_type().New();
-                    pRec->m_list.m_idOwner.store( curThreadId, atomics::memory_order_relaxed );
+                    pRec = allocator_type().New( curThreadId );
+                    CDS_COMPILER_RW_BARRIER;
 
                     thread_record * pOldHead = m_pHead.load( atomics::memory_order_acquire );
                     do {
                         pRec->m_list.m_pNext = pOldHead;
+                        // Compiler barrier: assignment above MUST BE inside the loop
+                        CDS_COMPILER_RW_BARRIER;
                     } while ( !m_pHead.compare_exchange_weak( pOldHead, pRec, atomics::memory_order_acq_rel, atomics::memory_order_acquire ));
 
                     return pRec;

diff --git a/cds/urcu/details/gp_decl.h b/cds/urcu/details/gp_decl.h
index 58c2bb8be5f0e4e54e3b4f10b30adaf72c44870e..ed4177c3fdbfff9e899b8b625d6e73e8e9b6c6a2 100644 (file)
--- a/cds/urcu/details/gp_decl.h
+++ b/cds/urcu/details/gp_decl.h
@@ -46,6 +46,7 @@ namespace cds { namespace urcu { namespace details {
         atomics::atomic<uint32_t>        m_nAccessControl ; \
         thread_list_record< thread_data >   m_list ; \
         thread_data(): m_nAccessControl(0) {} \
+        explicit thread_data( OS::ThreadId owner ): m_nAccessControl(0), m_list(owner) {} \
         ~thread_data() {} \
     }
 

diff --git a/cds/urcu/details/sh_decl.h b/cds/urcu/details/sh_decl.h
index e552962283cc102b13720678699b29d25e8e5182..81a7ea32a02c8d097054f38e4c7eb6aac24e0469 100644 (file)
--- a/cds/urcu/details/sh_decl.h
+++ b/cds/urcu/details/sh_decl.h
@@ -51,6 +51,7 @@ namespace cds { namespace urcu { namespace details {
         atomics::atomic<bool>            m_bNeedMemBar    ; \
         thread_list_record< thread_data >   m_list ; \
         thread_data(): m_nAccessControl(0), m_bNeedMemBar(false) {} \
+        explicit thread_data( OS::ThreadId owner ): m_nAccessControl(0), m_bNeedMemBar(false), m_list(owner) {} \
         ~thread_data() {} \
     }
 

diff --git a/src/hp_gc.cpp b/src/hp_gc.cpp
index ec8ecc2a01764c1853ec0bbc0bfebe294e331b99..08043f42e1932e9678f395a6fe2b102ac5d4d937 100644 (file)
--- a/src/hp_gc.cpp
+++ b/src/hp_gc.cpp
@@ -112,10 +112,10 @@ namespace cds { namespace gc {
             }
         }
 
-        inline GarbageCollector::hplist_node * GarbageCollector::NewHPRec()
+        inline GarbageCollector::hplist_node * GarbageCollector::NewHPRec( OS::ThreadId owner )
         {
             CDS_HAZARDPTR_STATISTIC( ++m_Stat.m_AllocNewHPRec )
-            return new hplist_node( *this );
+            return new hplist_node( *this, owner );
         }
 
         inline void GarbageCollector::DeleteHPRec( hplist_node * pNode )
@@ -144,13 +144,14 @@ namespace cds { namespace gc {
 
             // No HP records available for reuse
             // Allocate and push a new HP record
-            hprec = NewHPRec();
-            hprec->m_idOwner.store( curThreadId, atomics::memory_order_release );
-            hprec->m_bFree.store( false, atomics::memory_order_release );
+            hprec = NewHPRec( curThreadId );
+            CDS_COMPILER_RW_BARRIER;
 
             hplist_node * pOldHead = m_pListHead.load( atomics::memory_order_acquire );
             do {
                 hprec->m_pNextNode = pOldHead;
+                // Compiler barrier: assignment above MUST BE inside the loop
+                CDS_COMPILER_RW_BARRIER;
             } while ( !m_pListHead.compare_exchange_weak( pOldHead, hprec, atomics::memory_order_acq_rel, atomics::memory_order_acquire ));
 
             return hprec;

diff --git a/tools/tsan-suppression b/tools/tsan-suppression
index e51ad85095dd474d40b891251743b14c988ddaf5..71cc3de2471aa457237697f3f4c88de97846f7d5 100644 (file)
--- a/tools/tsan-suppression
+++ b/tools/tsan-suppression
@@ -3,6 +3,11 @@
 #   suppressions=<supression_file_name>
 #   verosity=n Verbosity level (0 - silent, 1 - a bit of output, 2+ - more output).
 #   history_size=[0..7], default 2
+#   detect_deadlocks=0 - some data structs in libcds tests use a lot of node-level mutexes.
+#                        TSan has the hardcoded limit =16 for the number of mutex per thread.
+#                        To prevent "possibly deadlock" reporting disable deadlock detection.
+#                        Suppression can help in that case but stack unwinding increases 
+#                        test time significantly.
 
 # false: LazyList potential deadlock
 deadlock:cds/intrusive/impl/lazy_list.h