summary |
shortlog |
log |
commit | commitdiff |
tree
raw |
patch |
inline | side by side (from parent 1:
97e63f0)
The code sample in Documentation/circular-buffers.txt appears to have a
few ordering bugs. This patch therefore applies the needed fixes.
Reported-by: Lech Fomicki <lfomicki@poczta.fm>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
smp_wmb(); /* commit the item before incrementing the head */
smp_wmb(); /* commit the item before incrementing the head */
- buffer->head = (head + 1) & (buffer->size - 1);
+ ACCESS_ONCE(buffer->head) = (head + 1) & (buffer->size - 1);
/* wake_up() will make sure that the head is committed before
* waking anyone up */
/* wake_up() will make sure that the head is committed before
* waking anyone up */
before the head index makes it available to the consumer and then instructs the
CPU that the revised head index must be written before the consumer is woken.
before the head index makes it available to the consumer and then instructs the
CPU that the revised head index must be written before the consumer is woken.
-Note that wake_up() doesn't have to be the exact mechanism used, but whatever
-is used must guarantee a (write) memory barrier between the update of the head
-index and the change of state of the consumer, if a change of state occurs.
+Note that wake_up() does not guarantee any sort of barrier unless something
+is actually awakened. We therefore cannot rely on it for ordering. However,
+there is always one element of the array left empty. Therefore, the
+producer must produce two elements before it could possibly corrupt the
+element currently being read by the consumer. Therefore, the unlock-lock
+pair between consecutive invocations of the consumer provides the necessary
+ordering between the read of the index indicating that the consumer has
+vacated a given element and the write by the producer to that same element.
if (CIRC_CNT(head, tail, buffer->size) >= 1) {
/* read index before reading contents at that index */
if (CIRC_CNT(head, tail, buffer->size) >= 1) {
/* read index before reading contents at that index */
- smp_read_barrier_depends();
/* extract one item from the buffer */
struct item *item = buffer[tail];
/* extract one item from the buffer */
struct item *item = buffer[tail];
smp_mb(); /* finish reading descriptor before incrementing tail */
smp_mb(); /* finish reading descriptor before incrementing tail */
- buffer->tail = (tail + 1) & (buffer->size - 1);
+ ACCESS_ONCE(buffer->tail) = (tail + 1) & (buffer->size - 1);
}
spin_unlock(&consumer_lock);
}
spin_unlock(&consumer_lock);
This prevents the compiler from discarding and reloading its cached value -
which some compilers will do across smp_read_barrier_depends(). This isn't
strictly needed if you can be sure that the opposition index will _only_ be
This prevents the compiler from discarding and reloading its cached value -
which some compilers will do across smp_read_barrier_depends(). This isn't
strictly needed if you can be sure that the opposition index will _only_ be
+used the once. Similarly, ACCESS_ONCE() is used in both algorithms to
+write the thread's index. This documents the fact that we are writing
+to something that can be read concurrently and also prevents the compiler
+from tearing the store.