rrnewton / haskell-lockfree

A collection of different packages for CAS based data structures.
106 stars 25 forks source link

Approximate GHC's old atomic barriers with C11 fences #89

Closed bgamari closed 9 months ago

bgamari commented 10 months ago

As of GHC 9.9 the old-style Cmm atomic barriers have been removed. We now approximate them with C11 atomic fences. I believe these approximations should be at least as strong as the previous barriers.

Addresses #88.

locallycompact commented 9 months ago

I tried this patch but it seems to be failing further up in client libraries.

polysemy>     /nix/store/xb2kx669mp3b4jm6p0q2hmqvydbffgwf-atomic-primops-0.8.4/lib/ghc-9.9/lib/x86_64-linux-ghc-9.9-inplace/libHSatomic-primops-0.8.4-EP9DoMaH0fnBUbuVh3pzzs-ghc9.9.so: undefined symbol: hs_atomics_primops_store_load_barrier

https://gitlab.horizon-haskell.net/package-sets/horizon-advance/-/jobs/1360474#L168

RyanGlScott commented 9 months ago

I believe that error is likely caused by the version bounds confusion observed in https://github.com/rrnewton/haskell-lockfree/pull/89#discussion_r1469468298. I just pushed a fix for this in https://github.com/rrnewton/haskell-lockfree/pull/89/commits/cdf331a1ba350f53661f906c9c4bb807528cb6b8. Can you try again with the latest version of the PR?

locallycompact commented 9 months ago

I did already have that modification actually

RyanGlScott commented 9 months ago

I tried to compile polysemy-1.9.1.3 against this PR's version of atomic-primops using GHC 9.9.20240210 (commit 8bbe12f288d2916c598cb72b87dfb602739ff80c), but I wasn't able to reproduce the linker error seen in https://gitlab.horizon-haskell.net/package-sets/horizon-advance/-/jobs/1360474#L168. I don't suppose this error relies on polysemy being compiled in a particular way? I tried using various combinations of flags from that job (--enable-profiling, --enable-shared, etc.), but I couldn't figure it out.

locallycompact commented 9 months ago

I don't know of anything that could affect it.

RyanGlScott commented 9 months ago

Ugh, I think I see what is happening. I managed to overlook this cabal warning when building atomic-primops:

Build profile: -w ghc-9.9.20240210 -O1
In order, the following will be built (use -v for more details):
 - atomic-primops-0.8.4 (lib) (first run)
Warning: atomic-primops.cabal:55:5: The field "cmm-sources" is available only
since the Cabal specification version 3.0. This field will be ignored.

That definitely looks like a problem. (The fact that GHC gets as far as it does without compiling atomics.cmm is rather surprising, but oh well.)

RyanGlScott commented 9 months ago

I've pushed a commit which updates atomic-primops.cabal to use cabal-version: 3.0, which is a prerequisite for using cmm-sources. Unfortunately, this now reveals a more serious issue: atomics.cmm simply doesn't compile:

cbits/atomics.cmm:8:1: error: [GHC-09848]
    unknown macro SEQ_CST_FENCE
  |
8 | }
  | ^

@bgamari, do you know what is going on here?

RyanGlScott commented 9 months ago

This error occurs because Cmm.h doesn't define a SEQ_CST_FENCE macro, unlike other memory barrier macros. @TerrorJack has implemented this macro in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12064, so I'm inclined to park this until that PR has landed.

RyanGlScott commented 9 months ago

https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12064 has landed upstream in GHC. That being said, I still get the same error as in https://github.com/rrnewton/haskell-lockfree/pull/89#issuecomment-1937801217 if I try to build atomic-primops using GHC HEAD. @bgamari, do you know why that would be?

I can fix the issue if I inline the macros:

diff --git a/atomic-primops/cbits/atomics.cmm b/atomic-primops/cbits/atomics.cmm
index 86da7cd..27cfdee 100644
--- a/atomic-primops/cbits/atomics.cmm
+++ b/atomic-primops/cbits/atomics.cmm
@@ -4,14 +4,14 @@
 // ordered atomic fences.

 hs_atomic_primops_store_load_barrier() {
-  SEQ_CST_FENCE();
+  prim %fence_seq_cst();
 }

 hs_atomic_primops_load_load_barrier() {
-  ACQUIRE_FENCE();
+  prim %fence_acquire();
 }

 hs_atomic_primops_write_barrier() {
-  RELEASE_FENCE();
+  prim %fence_release();
 }
TerrorJack commented 9 months ago

These FENCE macros are not function like macros, you need to strip the parens

TerrorJack commented 9 months ago

I took a closer look. The FENCE macros are only defined if THREADED_RTS is defined, but that macro is only defined by hadrian when compiling .cmm files in rts, and won't be defined for cmm-sources in third party packages. So it looks like cmm-sources should just use the %prim syntax directly.