From 61d3987a3cda1ab656a9224349d9aac7fbfb1fbc Mon Sep 17 00:00:00 2001 From: khizmax Date: Sun, 16 Apr 2017 12:21:53 +0300 Subject: [PATCH] Fixed missing acquire read in Flat-Combining algorithm (found by TSan) Removed redundant TSan annotation from FC-based containers --- cds/algo/flat_combining/kernel.h | 37 ++++++++++++++++++++++++++++++-- cds/container/fcdeque.h | 10 +-------- cds/container/fcpriority_queue.h | 4 ++-- cds/container/fcqueue.h | 10 +-------- cds/container/fcstack.h | 9 +++----- cds/intrusive/fcqueue.h | 10 ++++----- cds/intrusive/fcstack.h | 10 +-------- 7 files changed, 48 insertions(+), 42 deletions(-) diff --git a/cds/algo/flat_combining/kernel.h b/cds/algo/flat_combining/kernel.h index 30dcc7dc..202293b4 100644 --- a/cds/algo/flat_combining/kernel.h +++ b/cds/algo/flat_combining/kernel.h @@ -708,7 +708,7 @@ namespace cds { namespace algo { while ( p ) { switch ( p->nState.load( memory_model::memory_order_acquire )) { case active: - if ( p->op() >= req_Operation ) { + if ( p->op( memory_model::memory_order_acquire ) >= req_Operation ) { p->nAge.store( nCurAge, memory_model::memory_order_relaxed ); owner.fc_apply( static_cast( p )); operation_done( *p ); @@ -748,7 +748,7 @@ namespace cds { namespace algo { compact_list( nCurAge ); } - bool wait_for_combining( publication_record_type * pRec ) + bool wait_for_combining( publication_record_type* pRec ) { m_waitStrategy.prepare( *pRec ); m_Stat.onPassiveWait(); @@ -864,4 +864,37 @@ namespace cds { namespace algo { } // namespace flat_combining }} // namespace cds::algo +/* + CppMem model (http://svr-pes20-cppmem.cl.cam.ac.uk/cppmem/) + + // Combiner thread - slave (waiting) thread +int main() { + atomic_int y = 0; // pRec->op + int x = 0; // pRec->data + {{{ + { // slave thread (not combiner) + // Op data + x = 1; + // Annotate request (op) + y.store(1, release); + // Wait while request done + y.load(acquire).readsvalue(2); + // Read result + r2=x; + } + ||| + { // Combiner thread + // Read request (op) + r1=y.load(acquire).readsvalue(1); + // Execute request - change request data + x = 2; + // store "request processed" flag (pRec->op := req_Response) + y.store(2, release); + } + }}}; + return 0; +} + +*/ + #endif // #ifndef CDSLIB_ALGO_FLAT_COMBINING_KERNEL_H diff --git a/cds/container/fcdeque.h b/cds/container/fcdeque.h index 3270be7b..d081ee0e 100644 --- a/cds/container/fcdeque.h +++ b/cds/container/fcdeque.h @@ -381,9 +381,6 @@ namespace cds { namespace container { { assert( pRec ); - // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; - switch ( pRec->op()) { case op_push_front: assert( pRec->pValPush ); @@ -425,7 +422,6 @@ namespace cds { namespace container { assert(false); break; } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; } /// Batch-processing flat combining @@ -433,11 +429,8 @@ namespace cds { namespace container { { typedef typename fc_kernel::iterator fc_iterator; - // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; - for ( fc_iterator it = itBegin, itPrev = itEnd; it != itEnd; ++it ) { - switch ( it->op()) { + switch ( it->op( atomics::memory_order_acquire )) { case op_push_front: if ( itPrev != itEnd && (itPrev->op() == op_pop_front || (m_Deque.empty() && itPrev->op() == op_pop_back))) @@ -552,7 +545,6 @@ namespace cds { namespace container { break; } } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; } //@endcond diff --git a/cds/container/fcpriority_queue.h b/cds/container/fcpriority_queue.h index 83fd323b..97983ff2 100644 --- a/cds/container/fcpriority_queue.h +++ b/cds/container/fcpriority_queue.h @@ -280,7 +280,7 @@ namespace cds { namespace container { assert( pRec ); // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; + //CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; switch ( pRec->op()) { case op_push: @@ -308,7 +308,7 @@ namespace cds { namespace container { break; } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; + //CDS_TSAN_ANNOTATE_IGNORE_RW_END; } //@endcond }; diff --git a/cds/container/fcqueue.h b/cds/container/fcqueue.h index df86bda8..5c5d7647 100644 --- a/cds/container/fcqueue.h +++ b/cds/container/fcqueue.h @@ -315,9 +315,6 @@ namespace cds { namespace container { { assert( pRec ); - // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; - switch ( pRec->op()) { case op_enq: assert( pRec->pValEnq ); @@ -343,7 +340,6 @@ namespace cds { namespace container { assert(false); break; } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; } /// Batch-processing flat combining @@ -351,11 +347,8 @@ namespace cds { namespace container { { typedef typename fc_kernel::iterator fc_iterator; - // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; - for ( fc_iterator it = itBegin, itPrev = itEnd; it != itEnd; ++it ) { - switch ( it->op()) { + switch ( it->op( atomics::memory_order_acquire )) { case op_enq: case op_enq_move: case op_deq: @@ -368,7 +361,6 @@ namespace cds { namespace container { break; } } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; } //@endcond diff --git a/cds/container/fcstack.h b/cds/container/fcstack.h index 1b3aa45f..65994be7 100644 --- a/cds/container/fcstack.h +++ b/cds/container/fcstack.h @@ -301,8 +301,6 @@ namespace cds { namespace container { { assert( pRec ); - // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; switch ( pRec->op()) { case op_push: assert( pRec->pValPush ); @@ -331,18 +329,17 @@ namespace cds { namespace container { assert(false); break; } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; } /// Batch-processing flat combining void fc_process( typename fc_kernel::iterator itBegin, typename fc_kernel::iterator itEnd ) { // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; + //CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; typedef typename fc_kernel::iterator fc_iterator; for ( fc_iterator it = itBegin, itPrev = itEnd; it != itEnd; ++it ) { - switch ( it->op()) { + switch ( it->op( atomics::memory_order_acquire )) { case op_push: case op_push_move: case op_pop: @@ -353,7 +350,7 @@ namespace cds { namespace container { break; } } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; + //CDS_TSAN_ANNOTATE_IGNORE_RW_END; } //@endcond diff --git a/cds/intrusive/fcqueue.h b/cds/intrusive/fcqueue.h index 6fe82547..5ec67cd6 100644 --- a/cds/intrusive/fcqueue.h +++ b/cds/intrusive/fcqueue.h @@ -287,7 +287,7 @@ namespace cds { namespace intrusive { // this function is called under FC mutex, so switch TSan off // All TSan warnings are false positive - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; + //CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; switch ( pRec->op()) { case op_enq: @@ -311,7 +311,7 @@ namespace cds { namespace intrusive { assert(false); break; } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; + //CDS_TSAN_ANNOTATE_IGNORE_RW_END; } /// Batch-processing flat combining @@ -319,11 +319,11 @@ namespace cds { namespace intrusive { { // this function is called under FC mutex, so switch TSan off // All TSan warnings are false positive - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; + //CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; typedef typename fc_kernel::iterator fc_iterator; for ( fc_iterator it = itBegin, itPrev = itEnd; it != itEnd; ++it ) { - switch ( it->op()) { + switch ( it->op( atomics::memory_order_acquire )) { case op_enq: case op_deq: if ( m_Queue.empty()) { @@ -335,7 +335,7 @@ namespace cds { namespace intrusive { break; } } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; + //CDS_TSAN_ANNOTATE_IGNORE_RW_END; } //@endcond diff --git a/cds/intrusive/fcstack.h b/cds/intrusive/fcstack.h index 57d1fbd6..257079b0 100644 --- a/cds/intrusive/fcstack.h +++ b/cds/intrusive/fcstack.h @@ -259,7 +259,6 @@ namespace cds { namespace intrusive { return m_FlatCombining.statistics(); } - public: // flat combining cooperation, not for direct use! //@cond /// Flat combining supporting function. Do not call it directly! @@ -272,8 +271,6 @@ namespace cds { namespace intrusive { { assert( pRec ); - // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; switch ( pRec->op()) { case op_push: assert( pRec->pVal ); @@ -296,18 +293,14 @@ namespace cds { namespace intrusive { assert(false); break; } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; } /// Batch-processing flat combining void fc_process( typename fc_kernel::iterator itBegin, typename fc_kernel::iterator itEnd ) { - // this function is called under FC mutex, so switch TSan off - CDS_TSAN_ANNOTATE_IGNORE_RW_BEGIN; - typedef typename fc_kernel::iterator fc_iterator; for ( fc_iterator it = itBegin, itPrev = itEnd; it != itEnd; ++it ) { - switch ( it->op()) { + switch ( it->op( atomics::memory_order_acquire )) { case op_push: case op_pop: if ( itPrev != itEnd && collide( *itPrev, *it )) @@ -317,7 +310,6 @@ namespace cds { namespace intrusive { break; } } - CDS_TSAN_ANNOTATE_IGNORE_RW_END; } //@endcond -- 2.34.1