Stall in opal_fifo test

[nmorey]

Thank you for taking the time to submit an issue!

Background information

I'm packaging openmpi 2.1 for SUSE and end up hitting a bug (probably GCC's fault) in the test suite

What version of Open MPI are you using? (e.g., v1.10.3, v2.1.0, git branch name and hash, etc.)

OpenMPI 2.1.0

Describe how Open MPI was installed (e.g., from a source/distribution tarball, from a git clone, from an operating system distribution package, etc.)

Built using tarball from the website

Please describe the system on which you are running

• Operating system/version:
  openSUSE_Tumbleweed
  gcc (SUSE Linux) 6.3.1 20170202 [gcc-6-branch revision 245119]

• Computer hardware:
  i586

• Network type:

---

Details of the problem

Running the opal_fifo test (through make check) stalls.
After some debugging, it appears we end up into a broken loop here (opal_fifo.h:262)

       if (!opal_atomic_cmpset_ptr (&fifo->opal_fifo_tail.data.item, item, &fifo->opal_fifo_ghost)) {
            while (&fifo->opal_fifo_ghost == item->opal_list_next) {
                opal_atomic_rmb ();
            }

Looking into the generated assembly, it looks something like this

=> 0x08049675 <+357>:	cmp    %edi,%eax
   0x08049677 <+359>:	je     0x8049675 <thread_test+357>

which unless I'm mistaken means that GCC cached the value and doesn't load them from memory anymore.

The rmb used comes from gcc builtins.
Simply adding this:

diff --git a/opal/include/opal/sys/gcc_builtin/atomic.h b/opal/include/opal/sys/gcc_builtin/atomic.h
index 82b75f47d8..eea743503c 100644
--- a/opal/include/opal/sys/gcc_builtin/atomic.h
+++ b/opal/include/opal/sys/gcc_builtin/atomic.h
@@ -51,6 +51,9 @@ static inline void opal_atomic_mb(void)
 
 static inline void opal_atomic_rmb(void)
 {
+#if OPAL_ASSEMBLY_ARCH == OPAL_IA32
+    __asm__ __volatile__("": : :"memory");
+#endif
     __atomic_thread_fence (__ATOMIC_ACQUIRE);
 }

fixes the issue.

This really seems like a GCC bug, but I figured it might be worth notifying you.

[bosilca]

__ATOMIC_ACQUIRE is indeed supposed to guarantee an inter-thread happens-before constraint. Your fix seems indeed reasonable, but I wonder what is the impact of this addition on compilers that do not have this issue.

[nmorey]

It will prevent reordering around which might not be too bad.
It probably will have a perf impact.

I've open a bug at GCC and they seem to agree that something is wrong:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80640

They also suggested this:

Note that the source declares opal_list_next as

volatile struct opal_list_item_t *opal_list_next;

but the 'volatile' qualifier applies to the pointed-to struct, not the field itself. If written as

volatile struct opal_list_item_t *volatile opal_list_next;

[bosilca]

And I thought we were not even supposed to use volatile ? ;) They are indeed right, we could have been more specific. As you already have access to the compiler, can you validate that their proposed change fixes the issue ?

[nmorey]

Why are you not supposed to use volatile ?
I'll give it a try but it might have more side effects than the memory barrier.
I'm not familiar enough with the code to know if this will create a lot of memory traffic or not...

[bosilca]

Despite all expectations, adding volatile to the pointer itself lead to significantly better performance on my testing environment (E5-2650 v3). Consistently.
There results are from NetPIPE on 2 bytes.
before: 2 B 490401 times --> 37.855 Mbps in 422.664 nsecs
after: 2 B 632486 times --> 40.565 Mbps in 394.425 nsecs

I'll dig into it more over the weekend, and if confirmed I will create a PR.

[bwbarrett]

I don't know about making the code correct; it seems like we should be marking the accesses, not the data (ala Linus's rants), but that's probably a much bigger patch...