Commit 737ccb70 authored by Robin Cheng's avatar Robin Cheng Committed by Facebook GitHub Bot

Fix atomic memory orderings in RelaxedConcurrencyPriorityQueue.

Summary:
Two ordering issues:

setTreeNode cannot store the node as relaxed, because the node is newly allocated before storing, and if another thread gets the node pointer value, relaxed ordering cannot guarantee that the node has been fully initialized before it.

There are two ways this node can be read, one via optimisticReadValue, which loads with memory_order_acquired, which would now correctly synchronize with the release; the other is readValue, which I think was intended to be used only when the lock corresponding to the node is held, but there is one case on line 781 in forceInsert where we fail to acquire the lock but it looks like we want to retry unless we're out of capacity (on a best effort basis) and therefore we call readValue to check the capacity (on a best effort basis), so it looks as if memory_order_relaxed is okay, but it is not, because the node is a pointer that may not be fully initialized with relaxed ordering. So I'm changing that to memory_order_acquire as well.

In general it's not really valid to use relaxed ordering if the value is a pointer and the pointee is allocated on the fly.

Reviewed By: yfeldblum

Differential Revision: D22936002

fbshipit-source-id: 1c8a000b6033e4a81df5f9a5e850cfe350dbeb15
parent f711b660
...@@ -484,11 +484,11 @@ class RelaxedConcurrentPriorityQueue { ...@@ -484,11 +484,11 @@ class RelaxedConcurrentPriorityQueue {
} }
FOLLY_ALWAYS_INLINE Node* getList(const Position& pos) { FOLLY_ALWAYS_INLINE Node* getList(const Position& pos) {
return levels_[pos.level][pos.index].head.load(std::memory_order_relaxed); return levels_[pos.level][pos.index].head.load(std::memory_order_acquire);
} }
FOLLY_ALWAYS_INLINE void setTreeNode(const Position& pos, Node* t) { FOLLY_ALWAYS_INLINE void setTreeNode(const Position& pos, Node* t) {
levels_[pos.level][pos.index].head.store(t, std::memory_order_relaxed); levels_[pos.level][pos.index].head.store(t, std::memory_order_release);
} }
// Merge two sorted lists // Merge two sorted lists
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment