Fix release fence ordering in inner_enqueue() to prevent size_approx() race on AArch64

[cppmage]

Fixes #171

What changed

Move fence(memory_order_release) in inner_enqueue()'s CanAlloc
branch so it precedes both writes that publish a newly allocated block
(tailBlock_->next = newBlock and tailBlock = newBlock), instead of
only the second one. See #171 for the full root-cause
analysis and repro.

Also expanded the comment at that call site to note that
size_approx() (and any other Block::next-chain traversal) depends
on this ordering too, not just try_dequeue().

Testing

• Reproduced the original crash on AArch64 with the existing
  size_approx stress test run in a loop (1000 iterations).
• Confirmed the crash no longer reproduces after the fix under the
  same loop (1000 iterations).
• Note: the existing size_approx stress test has other pre-existing
  assertion failures, unrelated to this change and not addressed here.

[cameron314]

Thanks for looking into this. It does seem like a real bug.

[cameron314]

I think this comment was to justify the previous placement of the fence, and can be removed, since it clearly missed a case in its reasoning :)

[cameron314]

I think we still need a release fence before setting tailBlock itself? Otherwise anything reading tailBlock might not see the latest value of tailBlock->next, which would be the opposite bug.

[cppmage]

I think we still need a release fence before setting tailBlock itself? Otherwise anything reading tailBlock might not see the latest value of tailBlock->next, which would be the opposite bug.

Yeap, that is right. We need to guarantee that tailBlock's data was prepared before changing it.
I'll restore the second fence and clarify the comment accordingly.

[cameron314]

Note that at the time this code was written, TSan generated false positives with lock-free data structures, which is why it wasn't used. I should probably try running under TSan again at some point, it's come a long way since then.

[cppmage]

Note that at the time this code was written, TSan generated false positives with lock-free data structures, which is why it wasn't used. I should probably try running under TSan again at some point, it's come a long way since then.

Launched with clang thread sanitize, everything seems normal for me.