v3.0.x: opal/fifo: fix 128-bit atomic fifo on Power9 #5419
Conversation
This commit updates the atomic fifo code to fix a consistency issue observed on Power9 systems when builtin atomics are used. The cause was two things: 1) a missing write memory barrier in fifo push, and 2) a read ordering issue when reading the fifo head non-atomically. This commit fixes both issues and appears to correct then inconsistency. Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov> (cherry picked from commit 8b09010) Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
jsquyres
changed the title
There was a problem hiding this comment.
@hjelmn, one spot looks iffy (since there's an assignment to old_head.data.item (inside of opal_read_counted_pointer) followed by an asignment to a local variable item OF that value. If those two instructions happened in opposite order, we might have problems. Is the compiler free to re-arrange instructions before or after (or both) after inlining functions?
| old_head.data.counter = lifo->opal_lifo_head.data.counter; | ||
| opal_atomic_rmb (); | ||
| old_head.data.item = item = (opal_list_item_t*)lifo->opal_lifo_head.data.item; | ||
| opal_read_counted_pointer (&lifo->opal_lifo_head, &old_head); |
There was a problem hiding this comment.
Do you need an opal_atomic_rmb() here after the opal_read_counted_pointer(), or is there a read memory barrier even if that function gets inlined?
There was a problem hiding this comment.
Should be ok since there is a rmb in opal_read_counted_pointer and old_head.data.item is a data dependency for the next line. Shouldn't be re-ordered by the compiler.
There was a problem hiding this comment.
thanks for taking a second look.
looks good to me.
|
I tested and verified this fixes the issue on Power9. |
3 participants
This commit updates the atomic fifo code to fix a consistency issue
observed on Power9 systems when builtin atomics are used. The cause
was two things: 1) a missing write memory barrier in fifo push, and 2)
a read ordering issue when reading the fifo head non-atomically. This
commit fixes both issues and appears to correct then inconsistency.
Signed-off-by: Nathan Hjelm hjelmn@lanl.gov
(cherry picked from commit 8b09010)
Signed-off-by: Nathan Hjelm hjelmn@lanl.gov