Improve thread-safety in DART #252

devreal · 2017-01-25T09:09:51Z

Improve thread-safety of certain DART operations and provide an interface for initialization of a thread-safe runtime.

Collective allocation and communication are now marked as thread-safe given that they are not called on the same team concurrently.

Addresses #118

fmoessbauer

I cannot see any unit tests of the thread safety features. Our CI containers do have MPI libs with thread support enabled. For the sake of completeness I added the docker scripts to this branch.

devreal · 2017-01-26T13:30:39Z

I added three test-cases, most importantly testing concurrent memory allocation on different teams and simple concurrent put: https://github.com/dash-project/dash/blob/bug-118-threadsafety/dash/test/ThreadsafetyTest.cc

Feel free to shoot me more ideas you might have :)

fuchsto · 2017-01-25T09:42:41Z

dart-impl/base/include/dash/dart/base/atomic.h

+#define DASH_DART_BASE_ATOMIC_H_
+
+
+#define DART_FETCH_AND_ADD64(ptr, val)   __sync_fetch_and_add((int64_t *)(ptr), (val))


These definitions need a guard checking for availability of the atomic compiler extensions, something in the line of HAVE_SYNC_FETCH_AND_ADD.

For a from-scratch implementation, see:

https://github.com/siemens/embb/blob/master/base_c/src/atomicfunc_32.asm

https://github.com/siemens/embb/blob/master/base_c/src/atomicfunc_64.asm

Also, please reformat the code for 80 columns.

I added a guard flag and an unsafe alternative. CMake errors out if thread-support is requested but the builtins are not available.

I am aware of the EMBB code but I would like to avoid pulling in assembler code by all means because it is just not portable. Just think of moving to ARM... The __sync builtins are supported by all compilers we currently support. Ideally we would use the __atomic builtins or C11 atomics (at some point) but at least the latter is still not supported by the Intel compiler at this point.

I agree! We def' do not want to ship assembly (If we do some day: I'm actually way better at ARM asm than x86)

fuchsto · 2017-01-25T09:44:08Z

build.sh

@@ -59,6 +59,7 @@ rm -Rf $BUILD_DIR/*
                        -DDART_IF_VERSION=3.2 \
                        -DINSTALL_PREFIX=$HOME/opt/dash-0.3.0/ \
                        -DDART_IMPLEMENTATIONS=mpi \
+                        -DENABLE_THREADING=ON \


Please decide for either ENABLE_THREADING or ENABLE_MULTITHREADING.

I ended up with ENABLE_THREADSUPPORT, which imo describes the semantics best.

fuchsto · 2017-01-25T09:46:45Z

dart-if/v3.2/include/dash/dart/if/dart_types.h

+   * the underlying runtime.
+   */
+  DART_THREAD_MULTIPLE = 10
+} dart_thread_level_t;


Puns like "thread level" have my heartfelt support, but perhaps we should name it dart_thread_support_level_t?

fuchsto · 2017-01-25T09:52:09Z

dash/include/dash/algorithm/SUMMA.h

+>
+using check_summa_pattern_constraints =
+    typename std::enable_if<
+      summa_pattern_constraints<MatrixTypeA>::satisfied::value &&


Should be MatrixTypeA, MatrixTypeB, MatrixTypeC (see lines below)

fuchsto · 2017-01-25T09:58:29Z

dash/include/dash/algorithm/SUMMA.h

@@ -121,6 +121,27 @@ typedef dash::pattern_layout_properties<
            dash::pattern_layout_tag::linear
        > summa_pattern_layout_constraints;

+template<typename MatrixType>
+using summa_pattern_constraints =


This only works for the current N x N implementation of SUMMA. The apparently redundant original definition served as a reminder that pattern constraints may depend on each other.
This simplification is okay for now, but it should not be in a public namespace. Please move it to dash::internal or dash::detail.

fuchsto · 2017-01-25T10:00:52Z

dart-impl/base/include/dash/dart/base/atomic.h

+#define DART_FETCH_AND_INC32(ptr)        __sync_fetch_and_add((int32_t *)(ptr), 1)
+#define DART_FETCH_AND_INC16(ptr)        __sync_fetch_and_add((int16_t *)(ptr), 1)
+#define DART_FETCH_AND_INC8(ptr)         __sync_fetch_and_add((int8_t  *)(ptr), 1)
+#define DART_FETCH_AND_INCPTR(ptr)       __sync_fetch_and_add((void   **)(ptr), sizeof(**ptr))


sizeof(**ptr) should be sizeof(**(ptr)), same in line 34.

devreal · 2017-02-08T10:00:54Z

@fuchsto I think I addressed all of your points. Please review and let me know if you have any other points :)

fuchsto · 2017-02-08T11:28:46Z

@devreal On it, right after lunch!

fuchsto

Looks fine for me, but unit tests should be way more comprehensive.

fuchsto · 2017-02-09T19:26:49Z

dart-impl/base/include/dash/dart/base/atomic.h

+#define DART_DEC_AND_FETCH8(ptr)     \
+          __sync_sub_and_fetch((int8_t  *)(ptr), 1)
+#define DART_DEC_AND_FETCHPTR(ptr)   \
+          __sync_sub_and_fetch((void   **)(ptr), sizeof(**ptr))


Should be sizeof(**(ptr))
But don't bother, can fix this myself before merging.

fuchsto · 2017-02-09T19:38:31Z

dash/test/ThreadsafetyTest.cc

+          arr_all.deallocate();
+          ASSERT_EQ_U(arr_all.size(), 0);
+        }
+        arr_all.allocate(team_all.size(), dash::DistributionSpec<1>(), team_all);


Increasing the size of the dash::Array instance might improve the probability to catch synchronization bugs.

fuchsto · 2017-02-09T19:40:03Z

dash/test/ThreadsafetyTest.cc

@@ -0,0 +1,131 @@
+


There are no tests of the introduced atomic operations (DART_COMPARE_AND_SWAP* etc.), right?

Correct, mainly because they are only used internally and not exposed to the outside world. It seems I cannot include dash/dart/base/atomic.h in a test file.

devreal · 2017-02-10T10:42:22Z

I will try to think of and create some more unit tests that could trigger errors.

…n shared-memory get.

…malloc

…nto bug-118-threadsafety

devreal · 2017-02-13T13:12:20Z

I created some additional unit tests to cover dash::memalloc and DASH algorithms. Please review and merge if appropriate.

fuchsto · 2017-02-13T13:17:35Z

@devreal Splendid! Will merge as soon as CI is done.

fuchsto · 2017-02-13T14:19:53Z

Looks like the clang build in CI does not support OpenMP:

https://1203-45930061-gh.circle-artifacts.com/0/tmp/circle-junit.9IGEHUV/mpich/clang/Release/logs/build.log

Build system correctly reports the missing thread support:

-- Enable multithreading:    (ENABLE_THREADSUPPORT)          OFF

(While I'm at it: this line should be placed above, at the other ENABLE_ entries, and alignment of OFF is wrong)

This should be okay. The test case should be disabled if DASH / DART are built without OpenMP support, however.

…nto bug-118-threadsafety

devreal added the specification:dart-if label Jan 25, 2017

devreal assigned fuchsto, fuerlinger, fmoessbauer and devreal Jan 25, 2017

devreal requested review from fuchsto and fmoessbauer January 25, 2017 09:09

fmoessbauer requested changes Jan 25, 2017

View reviewed changes

fmoessbauer approved these changes Jan 26, 2017

View reviewed changes

fuchsto requested changes Feb 3, 2017

View reviewed changes

devreal mentioned this pull request Feb 6, 2017

Make segment IDs unique again! #257

Merged

devreal added 7 commits February 6, 2017 14:33

Check availability of __sync functions and provide unsafe alternatives

a5401c2

Use [DART|DASH]_ENABLE_THREADSUPPORT and DART_HAVE_PTHREADS guards

92c1802

dart_thread_level_t -> dart_thread_support_level_t

b5f0f95

Add missing CMake changes

e4c4842

Fix fall-back surrogates used if thread-support is disabled

f25d6f9

Remove dash::check_summa_pattern_constraints

d20ebf4

Fix path to dash-ci.sh script in CI documentation

c29ff99

fuchsto requested changes Feb 10, 2017

View reviewed changes

Merge branch 'development' into bug-118-threadsafety

591da1d

devreal and others added 6 commits February 10, 2017 11:53

Disable GCC warning for EXPECT_EQ_U

188fb6b

Merge branch 'development' into bug-118-threadsafety

a96f3ce

Fix debug output for thread-support status

94c47f5

Fix compiler warning about initialization ordering

378f014

Add trace output for team create

77a3280

Extend thread-safety tests to include parallel mem register

293e510

devreal added 7 commits February 13, 2017 10:06

BUG: Fix an issue with the conversion from global to local unit IDs i…

8a265c0

…n shared-memory get.

Add dash::memfree and check return value of dart_memalloc in dash::me…

88ae3c0

…malloc

Add test for concurrent calls to dart_memalloc

5d58ffa

Fix fix for team-based unit IDs in shared memory get

196a60f

Add test case for concurrent dash::Algorithm invocation

555773f

Merge branch 'bug-118-threadsafety' of github.com:dash-project/dash i…

20061d0

…nto bug-118-threadsafety

Fix braces in DART_FETCH_AND_DECPTR

5ba4ce2

Merge branch 'development' into bug-118-threadsafety

4e6e081

fuchsto approved these changes Feb 13, 2017

View reviewed changes

Merge branch 'development' into bug-118-threadsafety

76e59cc

devreal added 2 commits February 13, 2017 15:47

Use DASH_ENABLE_OPENMP to guard OpenMP statements

459d416

Merge branch 'bug-118-threadsafety' of github.com:dash-project/dash i…

eba464a

…nto bug-118-threadsafety

fuchsto merged commit 9befaef into development Feb 13, 2017

fuchsto deleted the bug-118-threadsafety branch February 13, 2017 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve thread-safety in DART #252

Improve thread-safety in DART #252

devreal commented Jan 25, 2017

fmoessbauer left a comment

devreal commented Jan 26, 2017

fuchsto Jan 25, 2017

devreal Feb 6, 2017

fuchsto Feb 8, 2017

fuchsto Jan 25, 2017

devreal Feb 6, 2017

fuchsto Feb 8, 2017

fuchsto Jan 25, 2017

fuchsto Jan 25, 2017

fuchsto Jan 25, 2017

fuchsto Jan 25, 2017

devreal commented Feb 8, 2017

fuchsto commented Feb 8, 2017

fuchsto left a comment

fuchsto Feb 9, 2017

fuchsto Feb 9, 2017

fuchsto Feb 9, 2017

devreal Feb 10, 2017

devreal commented Feb 10, 2017

devreal commented Feb 13, 2017

fuchsto commented Feb 13, 2017

fuchsto commented Feb 13, 2017 •

edited

Loading

		#define DASH_DART_BASE_ATOMIC_H_


		#define DART_FETCH_AND_ADD64(ptr, val) __sync_fetch_and_add((int64_t *)(ptr), (val))

Improve thread-safety in DART #252

Improve thread-safety in DART #252

Conversation

devreal commented Jan 25, 2017

fmoessbauer left a comment

Choose a reason for hiding this comment

devreal commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devreal commented Feb 8, 2017

fuchsto commented Feb 8, 2017

fuchsto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devreal commented Feb 10, 2017

devreal commented Feb 13, 2017

fuchsto commented Feb 13, 2017

fuchsto commented Feb 13, 2017 • edited Loading

fuchsto commented Feb 13, 2017 •

edited

Loading