diff --git a/containers_cpp/include/embb/containers/internal/llx_scx-inl.h b/containers_cpp/include/embb/containers/internal/llx_scx-inl.h index a867344..1375cbe 100644 --- a/containers_cpp/include/embb/containers/internal/llx_scx-inl.h +++ b/containers_cpp/include/embb/containers/internal/llx_scx-inl.h @@ -99,15 +99,23 @@ bool LlxScx::TryLoadLinked( // Order of initialization matters: volatile bool marked_1 = data_record->IsMarkedForFinalize(); ScxRecord_t * curr_scx = data_record->ScxInfo().Load(); + // Guard active SCX record of this data record using guard 1. + // When calling help with this SCX record, it will be guarded + // using guard 0 again. + // This hazard pointer is validated in the nested if-block below. + hp.GuardPointer(1, curr_scx); volatile OperationState curr_state = curr_scx->State(); - bool marked_2 = data_record->IsMarkedForFinalize(); + bool marked_2 = data_record->IsMarkedForFinalize(); if (curr_state == OperationState::Aborted || (curr_state == OperationState::Comitted && !marked_2)) { // read mutable fields into local variable: UserData user_data_local(data_record->Data()); if (data_record->ScxInfo().Load() == curr_scx) { - // store in - // the thread-specific table: + // Active SCX record of data record did not change. + // Store in + // the thread-specific table. + // LLX results do not need to be guarded as they local to the + // thread. LlxResult llx_result; llx_result.data_record = data_record; llx_result.scx_record = curr_scx; @@ -118,16 +126,20 @@ bool LlxScx::TryLoadLinked( return true; } } + // Active SCX record of data record has been changed in between if (marked_1 && (curr_scx->State() == OperationState::Comitted || (curr_scx->State() == OperationState::InProgress && Help(curr_scx)))) { - // Successfully completed active SCX: + // Successfully completed the data record's active SCX but failed to + // complete the LLX operation because the data record has been finalized: finalized = true; return false; } if (data_record->ScxInfo().Load()->State() == OperationState::InProgress) { - // Help active SCX: - Help(data_record->ScxInfo().Load()); + // Help active SCX. + // This SCX record has been guarded above. + ScxRecord_t * data_record_scx = data_record->ScxInfo().Load(); + Help(data_record_scx); } return false; } @@ -157,6 +169,7 @@ bool LlxScx::TryStoreConditional( // Let info_fields be a table in shared memory containing for each r in V, // a copy of r's info value in this threads local table of LLX results. // Will be freed in Help() once the SCX operation has been completed. + // In brief: A list of the SCX record of all linked deps. scx_op_list_t * info_fields = scx_record_list_pool_.Allocate(max_links_); if (info_fields == NULL) { EMBB_THROW(embb::base::ErrorException, @@ -173,7 +186,7 @@ bool LlxScx::TryStoreConditional( llx_result_list::iterator llx_result_it; llx_result_list::iterator llx_result_end; llx_result_end = thread_llx_results_[thread_id]->end(); - // Find LLX result of r in thread-local LLX results: + // Find LLX result of data_record (r) in thread-local LLX results: for (llx_result_it = thread_llx_results_[thread_id]->begin(); llx_result_it != llx_result_end && llx_result_it->data_record != *it; @@ -193,6 +206,9 @@ bool LlxScx::TryStoreConditional( // Announce SCX operation. Lists linked_deps and finalize_dep are // guaranteed to remain on the stack until this announced operation // is completed, so no allocation/pool is necessary. + // The SCX operation description must be allocated from a pool as + // LLX data records might reference it after this operation has been + // completed. ScxRecord_t new_scx( linked_deps, finalize_deps, @@ -202,17 +218,21 @@ bool LlxScx::TryStoreConditional( cas_value, // old value: cas_field->Load(), - // linked SCX operations: + // list of the SCX record of all linked deps info_fields, // initial operation state: OperationState::InProgress); // Allocate from pool as this operation description is global: ScxRecord_t * scx = scx_record_pool_.Allocate(new_scx); + // Try to complete the operation. It will also be helped in failing + // TryLoadLinked operations. bool result = Help(scx); - hp.GuardPointer(0, NULL); - hp.GuardPointer(1, NULL); + // Release all load-links this SCX operation depended on ClearLinks(); - hp.EnqueuePointerForDeletion(scx); + // Release guards, but do not enqueue instance scx for deletion as it + // is still referenced in data records. + hp.GuardPointer(0, NULL); + hp.GuardPointer(1, NULL);; return result; } @@ -247,7 +267,9 @@ bool LlxScx::Help( ScxRecord * rinfo_old = *scx_op_it; hp.GuardPointer(1, rinfo_old); // Try to freeze the data record by setting its SCX info field - // to this SCX operation description: + // to this SCX operation description. + // r->ScxInfo() is not an ABA hazard as it is local to instance scx + // which is already guarded. if (r->ScxInfo().CompareAndSwap(rinfo_old, scx)) { // Do not try to delete the sentinel scx record: if (rinfo_old != &DataRecord_t::DummyScx) { @@ -279,6 +301,8 @@ bool LlxScx::Help( } // update CAS: cas_t expected_old_value = scx->old_value_; + // scx->old_value_ is not an ABA hazard as it is local to the instance + // scx which is already guarded. scx->field_->CompareAndSwap(expected_old_value, scx->new_value_); // Commit step. // Finalizes all r in data_records within finalize range and diff --git a/containers_cpp/test/llx_scx_test.cc b/containers_cpp/test/llx_scx_test.cc index 014272e..8328508 100644 --- a/containers_cpp/test/llx_scx_test.cc +++ b/containers_cpp/test/llx_scx_test.cc @@ -63,28 +63,30 @@ void LlxScxTest::ParallelTest() { int return_val = embb_internal_thread_index(&thread_index); if (return_val != EMBB_SUCCESS) EMBB_THROW(embb::base::ErrorException, "Could not get thread id!"); - // Threads try to append n nodes to a linked list in parallel - for (char value = 'a'; value <= 'p';) { + // Every thread adds every character from 'a' to 'z' into an ordered + // linked list + for (char value = 'a'; value <= 'z';) { // Find node to append new element on: - internal::LlxScxRecord * node = &head_llx; - internal::LlxScxRecord * next = node->Data().next_; - while (next != 0 && next->Data().value_ < value) { - node = next; - next = next->Data().next_; + internal::LlxScxRecord * node_rec = &head_llx; + internal::LlxScxRecord * next_rec = node_rec->Data().next_; + while (next_rec != 0 && next_rec->Data().value_ < value) { + node_rec = next_rec; + next_rec = next_rec->Data().next_; } - Node n; + Node node; bool finalized; // LLX on node the new element will be appended to: - if (!llxscx_.TryLoadLinked(node, n, finalized)) { + if (!llxscx_.TryLoadLinked(node_rec, node, finalized)) { continue; } - if (n.next_ == next) { + PT_ASSERT_MSG(!finalized, "No node should be finalized"); + if (node.next_ == next_rec) { // Pointer still valid after LLX, try to append new node internal::FixedSizeList *> linked_deps(1); internal::FixedSizeList *> finalize_deps(0); - linked_deps.PushBack(node); + linked_deps.PushBack(node_rec); // Create new node: - Node new_node(static_cast(thread_index), value, next); + Node new_node(static_cast(thread_index), value, next_rec); internal::LlxScxRecord * new_node_ptr = new internal::LlxScxRecord(new_node); // Convert node pointer to size_t: @@ -92,7 +94,7 @@ void LlxScxTest::ParallelTest() { // Convert target field pointer to size_t*: embb::base::Atomic * field_cas_ptr = reinterpret_cast< embb::base::Atomic * >( - &(node->Data().next_)); + &(node_rec->Data().next_)); // Call SCX: bool element_inserted = llxscx_.TryStoreConditional( @@ -119,7 +121,9 @@ void LlxScxTest::ParallelTestPost() { node = next; next = next->Data().next_; } - PT_ASSERT_EQ_MSG(static_cast(16 * num_threads_), values.size(), + // Check if every character from 'a' to 'z' has been added to the + // linked list in the correct order + PT_ASSERT_EQ_MSG(static_cast(26 * num_threads_), values.size(), "Unexpected size of result list"); }