Add atomic increment to FifoControllers #575
Conversation
This CL adds an increment counter methods that allow for atomically incrementing the read/write indexes, rather than geting the current index, adding to it and setting it. Also, getFullFramesAvailable() and getEmptyFramesAvailable() now clamp negative values to 0.
src/fifo/FifoController.h
Outdated
| @@ -32,19 +32,24 @@ class FifoController : public FifoControllerBase | |||
| FifoController(uint32_t bufferSize, uint32_t threshold); | |||
| virtual ~FifoController() = default; | |||
|
|
|||
| // TODO review use atomics or memory barriers | |||
There was a problem hiding this comment.
From what I could tell, the current use of memory barriers is adequate to keep mWriteCounter and mReadCounter consistent. However, I don't know if there could be problems in FifoControllerBase::getFullFramesAvailable(), when we access both variables. I can't tell if there is a scenario where two threads would get a different number of frames, that could be prevented by a use of std::memory_order_seq_cst instead. From here (https://en.cppreference.com/w/cpp/atomic/memory_order#Sequentially-consistent_ordering) :
"Sequential ordering may be necessary for multiple producer-multiple consumer situations where all consumers must observe the actions of all producers occurring in the same order."
How many producers/consumers can FifoBuffer have?
There was a problem hiding this comment.
Useful video:
https://youtu.be/ZQFzMfHIxng?t=3117
There was a problem hiding this comment.
This FIFO has only one reader and one writer.
| @@ -38,8 +38,8 @@ uint32_t FifoControllerBase::getReadIndex() const { | |||
| return static_cast<uint32_t>(getReadCounter() % mTotalFrames); | |||
| } | |||
|
|
|||
| void FifoControllerBase::advanceReadIndex(int numFrames) { | |||
| setReadCounter(getReadCounter() + numFrames); | |||
| void FifoControllerBase::advanceReadIndex(uint32_t numFrames) { | |||
There was a problem hiding this comment.
I'm not sure if the int --> uint32_t conversion is invalid here as well. If the int is there for a reason, does the write index also need an int?
This CL adds an increment counter methods that allow for atomically
incrementing the read/write indexes, rather than geting the current
index, adding to it and setting it.
Also, getFullFramesAvailable() and getEmptyFramesAvailable() now clamp
negative values to 0.