From 5cce473e0101b429d2fc61e0cc1716a1ddd98150 Mon Sep 17 00:00:00 2001 From: Hayodea Hekol Date: Wed, 26 Nov 2025 00:11:40 -0400 Subject: [PATCH] PcloudStimProd: make sh_ptrs to Pcloud*StimBuff atomic<> This change is a bit pedantic, but since these vars aren't accessed in any hotpath, it's fine to be pedantic. We made these sh_ptrs atomic so we can use acquire and release side effects when loading and storing them. This doesn't eliminate the problem of seeing inconsistent state across microcontrollers, but it helps with simple accesses like these ones we already do. --- .../openClCollatingAndMeshingEngine.cpp | 9 +++- .../livoxGen1/pcloudStimulusProducer.cpp | 53 ++++++++++++------- .../livoxGen1/pcloudStimulusProducer.h | 8 +-- todo | 7 --- 4 files changed, 46 insertions(+), 31 deletions(-) diff --git a/stimBuffApis/livoxGen1/openClCollatingAndMeshingEngine.cpp b/stimBuffApis/livoxGen1/openClCollatingAndMeshingEngine.cpp index 5b382b7..2714371 100644 --- a/stimBuffApis/livoxGen1/openClCollatingAndMeshingEngine.cpp +++ b/stimBuffApis/livoxGen1/openClCollatingAndMeshingEngine.cpp @@ -650,9 +650,14 @@ bool OpenClCollatingAndMeshingEngine::setupCollateDgramsArgs( // Set ambienceHighVal argument (arg 4) uint32_t ambienceHighVal = 0; - if (ambienceStimFrame.has_value() && parent.ambienceStimulusBuffer) { - ambienceHighVal = parent.ambienceStimulusBuffer->ambienceHighVal; + std::shared_ptr ambienceBuff = nullptr; + if (ambienceStimFrame.has_value() + && (ambienceBuff = parent.ambienceStimulusBuffer.load( + std::memory_order_acquire))) + { + ambienceHighVal = ambienceBuff->ambienceHighVal; } + err = clSetKernelArg(collateKernel, 4, sizeof(uint32_t), &ambienceHighVal); if (err != CL_SUCCESS) { diff --git a/stimBuffApis/livoxGen1/pcloudStimulusProducer.cpp b/stimBuffApis/livoxGen1/pcloudStimulusProducer.cpp index 1b9e4ed..cb22e56 100644 --- a/stimBuffApis/livoxGen1/pcloudStimulusProducer.cpp +++ b/stimBuffApis/livoxGen1/pcloudStimulusProducer.cpp @@ -214,12 +214,24 @@ void PcloudStimulusProducer::destroyAttachedStimulusBuffer( this->stop(); // Clear specialized buffer pointers if they match - if (meshStimulusBuffer == buffer) - { meshStimulusBuffer.reset(); } - if (intensityStimulusBuffer == buffer) - { intensityStimulusBuffer.reset(); } - if (ambienceStimulusBuffer == buffer) - { ambienceStimulusBuffer.reset(); } + auto meshBuff = meshStimulusBuffer.load(std::memory_order_acquire); + if (meshBuff == buffer) + { + meshBuff.reset(); + meshStimulusBuffer.store(nullptr, std::memory_order_release); + } + auto intensityBuff = intensityStimulusBuffer.load(std::memory_order_acquire); + if (intensityBuff == buffer) + { + intensityBuff.reset(); + intensityStimulusBuffer.store(nullptr, std::memory_order_release); + } + auto ambienceBuff = ambienceStimulusBuffer.load(std::memory_order_acquire); + if (ambienceBuff == buffer) + { + ambienceBuff.reset(); + ambienceStimulusBuffer.store(nullptr, std::memory_order_release); + } // Call base class implementation to remove from attachedStimulusBuffers StimulusProducer::destroyAttachedStimulusBuffer(buffer); @@ -266,7 +278,7 @@ std::cout << __func__ << ": $$$$$$$ Creating MeshStimulusBuffer" << std::endl; std::cout << __func__ << ": $$$$$$$ Created MeshStimulusBuffer" << std::endl; this->stop(); addAttachedStimulusBufferIfNotExists(meshBuffer); - meshStimulusBuffer = meshBuffer; + meshStimulusBuffer.store(meshBuffer, std::memory_order_release); this->start(); return meshBuffer; } @@ -289,7 +301,8 @@ std::cout << __func__ << ": $$$$$$$ Creating PcloudIntensityStimulusBuffer" << s std::cout << __func__ << ": $$$$$$$ Created PcloudIntensityStimulusBuffer" << std::endl; this->stop(); addAttachedStimulusBufferIfNotExists(intensityBuffer); - intensityStimulusBuffer = intensityBuffer; + intensityStimulusBuffer.store( + intensityBuffer, std::memory_order_release); this->start(); return intensityBuffer; } @@ -322,7 +335,7 @@ std::cout << __func__ << ": $$$$$$$ Created PcloudIntensityStimulusBuffer" << st std::cout << __func__ << ": $$$$$$$ Created PcloudAmbienceStimulusBuffer" << std::endl; this->stop(); addAttachedStimulusBufferIfNotExists(ambienceBuffer); - ambienceStimulusBuffer = ambienceBuffer; + ambienceStimulusBuffer.store(ambienceBuffer, std::memory_order_release); this->start(); return ambienceBuffer; } @@ -407,13 +420,14 @@ public: context->frameAssemblyResult = loop; // Check if intensity buffer is attached and acquire frame if so - if (pcloudProducer.intensityStimulusBuffer) + if (auto intensityBuff = pcloudProducer.intensityStimulusBuffer.load( + std::memory_order_acquire)) { - size_t intensityRingbuffIndex = pcloudProducer - .intensityStimulusBuffer->ringBuffer.getIndexToProduceInto(); + size_t intensityRingbuffIndex = intensityBuff + ->ringBuffer.getIndexToProduceInto(); - StimulusFrame& intensityStimFrame = pcloudProducer - .intensityStimulusBuffer->ringBuffer.getDataAtSlot( + StimulusFrame& intensityStimFrame = intensityBuff + ->ringBuffer.getDataAtSlot( intensityRingbuffIndex); intensityStimFrame.lock.writeAcquire(); @@ -425,13 +439,14 @@ public: } // Check if ambience buffer is attached and acquire frame if so - if (pcloudProducer.ambienceStimulusBuffer) + if (auto ambienceBuff = pcloudProducer.ambienceStimulusBuffer.load( + std::memory_order_acquire)) { - size_t ambienceRingbuffIndex = pcloudProducer - .ambienceStimulusBuffer->ringBuffer.getIndexToProduceInto(); + size_t ambienceRingbuffIndex = ambienceBuff + ->ringBuffer.getIndexToProduceInto(); - StimulusFrame& ambienceStimFrame = pcloudProducer - .ambienceStimulusBuffer->ringBuffer.getDataAtSlot( + StimulusFrame& ambienceStimFrame = ambienceBuff + ->ringBuffer.getDataAtSlot( ambienceRingbuffIndex); ambienceStimFrame.lock.writeAcquire(); diff --git a/stimBuffApis/livoxGen1/pcloudStimulusProducer.h b/stimBuffApis/livoxGen1/pcloudStimulusProducer.h index 3940210..103a2a1 100644 --- a/stimBuffApis/livoxGen1/pcloudStimulusProducer.h +++ b/stimBuffApis/livoxGen1/pcloudStimulusProducer.h @@ -91,9 +91,11 @@ public: StagingBuffer collationBuffer; size_t tempStimulusFrameMem; StimulusFrame tempStimulusFrame; - std::shared_ptr meshStimulusBuffer; - std::shared_ptr intensityStimulusBuffer; - std::shared_ptr ambienceStimulusBuffer; + std::atomic> meshStimulusBuffer; + std::atomic> + intensityStimulusBuffer; + std::atomic> + ambienceStimulusBuffer; private: class ProduceFrameReq; diff --git a/todo b/todo index f578ff6..7e85414 100644 --- a/todo +++ b/todo @@ -99,10 +99,3 @@ cancelation problem described above, concerning StimulusBuffer::start/stop(), and ensuring that after stop() has returned, we can be reasonably sure that all in-flight ops have exited. - - Making sh_ptr atomic for mem barriers: -We could also complete our implemetation's correctness by converting -the sh_ptrs to StimulusBuffer inside of the PCloudStimulusProducer -into std::atomic>, and using -std::memory_order_release/memory_order_acquire when writing and -reading them respectively.