Code review comment for lp:~mixxxdevelopers/mixxx/atomic-co

> > - Why is cReaderSlotCnt the number of concurrent readers allowed also the size of the ring buffer? I can't think of a reason why those 2 values need to be the same.

> In worst case, each reader can consume a reader slot from a different ring element. In this case there is still one ring element available for writing.

If I understand this right, every slot in the ring buffer can support cReaderSlotCnt readers. (i.e. reads fail after cReaderSlotCnt readers are active). When reads fail, a reader moves backward one in the ring buffer to the previous value to read.

So, worst case scenario (for a write), cReaderSlotCnt*cReaderSlotCnt+1 concurrent reads are occurring and there are no available write slots. I don't see how anything related to cReaderSlotCnt preserves an extra slot for writing? We could just have more readers.

> I want to have 8 ring element because it is a 2^x value.
> To be sure we are lock free, we need on ring element for each accessing thread. I have not counted CO accessing threads but I think 8 should not be reached. We should verify this.
> But It should also work with more tasks, because it is very unlikely that 8 threads are in the critical section at the same time. And when it relay happens, the additional threads are locked.

So, to characterize the scenarios when a thread spin-locks:

When (cReaderCnt+1)*cReaderCnt concurrent reads are occurring, then a reader will be spin-locked.
When cReaderCnt+1 concurrent writes are occurring, then a reader will be spin-locked.

When cReaderCnt*cReaderCnt + 1 concurrent reads are occurring, then a writer will be spin-locked.
When cReaderCnt+1 concurrent writes are occurring, then a writer will be spin-locked.

(there is no actual locking, just busy-waiting).

> > - When getValue() fails to read a slot, it goes backward one. This means we sacrifice consistency for speed. We need to make sure people understand this is a trade-off that is part of the control system. Under high load, the control system will possibly serve a stale result when you get().

> If there are enough ring elements this should not happen. The value is not stale, because the more recent value is not entirely written.

This can happen under excessive read-pressure (> cReaderSlotCnt concurrent reads) alone so I don't think that's true... (unless I'm missing something about how this works).

I don't see why cReaderSlotCnt and the ring buffer size need to be related. You could just pick cReaderSlotCnt as INT_MAX and make the ring buffer size 8 and we would effectively never have inconsistent reads under read pressure alone. Isn't cReaderSlotCnt just an arbitrary value we count down to 0 from?