Fix Argobots support #4421

shintaro-iwasaki · 2020-03-30T02:19:07Z

Pull Request Description

This PR fixes Argobots support.

Fix mpl/include/mpl_thread_argobots.h
This change will not affect the default Pthreads behavior.
Call MPID_Thread_init() in MPI_T_init_thread()
Any MPID_Thread_ operations should be called after MPID_Thread_init(). This will not affect the default Pthreads behavior since the Pthreads version does nothing in MPID_Thread_init().
- Note MPI_T_init_thread() can be called before MPI_Init_thread(), so MPI_T_init_thread() needs its own MPID_Thread_init().
Add yield() to a busy loop in test/mpi/threads/comm/idup_deadlock
Nonpreemptive threads (e.g., Argobots threads) can cause a deadlock if there is a busy loop. This PR adds a new yield function (MTest_thread_yield()) and inserts it into a busy loop in test/mpi/threads/comm/idup_deadlock.
This will not affect the default Pthreads behavior since the Pthreads version does nothing in MTest_thread_yield().

Note

Regarding 3. MTest_thread_yield(), there are many options.

Add yield
1.a. [What this PR does] Add MTest_thread_yield(), which calls yield() only when the underlying thread is nonpreemptive.
1.b. Add MTest_thread_scheduling_point() (instead of MTest_thread_yield()). The functionality is the same as that of 1.a.
1.c. Add MTest_thread_yield(), which calls yield() even with Pthreads (e.g., pthread_yield()).
Add another synchronization method that has a scheduling point when the underlying thread is nonpreemptive (e.g., MTest_thread_lock()).

Expected Impact

No performance difference for the default Pthreads version.

Author Checklist

Reference appropriate issues (with "Fixes" or "See" as appropriate)
Fixes test/mpich/threads: MTest_init_thread_pkg() is not called. #4422.
Remove xfail from the test suite when fixing a test
Commits are self-contained and do not do two things at once
Commit message is of the form: module: short description and follows good practice
Passes whitespace checkers
Passes warning tests
Passes all tests
Add comments such that someone without knowledge of the code could understand
Add Devel Docs in the doc/ directory for any new code design

shintaro-iwasaki · 2020-03-30T02:20:58Z

test:jenkins/ch3/tcp

shintaro-iwasaki · 2020-03-30T02:36:20Z

test:jenkins/ch4/argobots

shintaro-iwasaki · 2020-03-30T09:22:59Z

test:jenkins/ch4/ofi

shintaro-iwasaki · 2020-03-30T10:01:17Z

The Argobots test failure is caused by an mtest issue. I created a new issue #4422
Once #4422 is solved, it should work well; at least the Argobots version works in my testing environment.

src/mpl/include/mpl_thread_argobots.h

hzhou · 2020-03-30T13:14:07Z

src/include/mpitimpl.h

@@ -1192,6 +1192,8 @@ extern MPID_Thread_mutex_t mpi_t_mutex;
    do { \
        int err_; \
        MPIR_T_THREAD_CHECK_BEGIN \
+        MPID_Thread_init(&err_); \


Can argobot init be called multiple times?

Yes. It is okay to call it multiple times (internally there's a refcount).
If it wouldn't be the case, MPL_thread_init() will take care of it.

This concern is mentioned in src/mpl/include/mpl_thread_argobots.h.

src/include/mpitimpl.h

shintaro-iwasaki · 2020-03-30T13:35:56Z

I added test: use refcount to initialize/finalize Argobots runtime, which addresses #4422 by adopting the solution 0 (removing MTest_init_thread_pkg() from Argobots).

shintaro-iwasaki · 2020-03-30T13:36:29Z

test:jenkins/ch4/argobots

shintaro-iwasaki · 2020-03-30T14:55:00Z

test:jenkins/ch4/argobots
test:jenkins/ch4/ofi

hzhou · 2020-03-30T15:02:08Z

test/mpi/util/mtest_thread_abt.h

+/* Reference counter */
+
+static void MTest_abt_incr_refcount(void);
+static void MTest_abt_decr_refcount(void);


I understand this is only included in mtest_thread.c, but it is concerning to have static implementation inside a .h header.

In terms of coding practice, I believe non-static/inline definitions should be avoided since it often causes a "multiple definition" error.

In this specific case, static means these functions cannot be called by individual tests.

hzhou · 2020-03-30T15:07:07Z

test/mpi/util/mtest_thread_abt.h

+
+static void MTest_abt_incr_refcount(void)
+{
+    MTest_abt_acquire_spinlock();


Will it work without the lock?

Lock initialization might happen in created threads, so this spinlock is necessary.

I think this is beyond simple. It adds the lazy initialization logic, then the concern of checking and initialization inside the spawned threads, then the concern of using spin-lock, and then the concern of atomics. I much prefer solution #2 at this point.

Thank you. I got it. I will adopt solution #2.

This has been fixed by #4429, so this PR no longer contains it.

shintaro-iwasaki · 2020-04-08T15:05:24Z

test:mpich/ch4/argobots
test:mpich/ch4/ofi

shintaro-iwasaki · 2020-04-08T17:25:27Z

test:mpich/ch4/argobots

shintaro-iwasaki · 2020-04-10T22:56:44Z

test:mpich/ch4/argobots

When multithreaded MPI is enabled, MPI_T_init_thread() needs to initialize a mutex, mpi_t_mutex. This mutex should be created after initializing MPL/thread, which is especially necessary when the underlying threads are not Pthreads (e.g., Argobots threads). This patch adds initialization and finalization of MPL/thread in MPI_T_init_thread() and MPI_T_finalize().

When underlying threads are nonpreemptive (e.g., Argobots threads), a busy loop can cause a deadlock. This patch fixes a busy loop in threads/idup_deadlock by adding MTest_thread_yield(), a new function that is translated into yield when threads are nonpreemptive.

The default stack size of Argobots threads is too small and can cause a stack overflow bug. This patch fixes it.

shintaro-iwasaki · 2020-04-11T15:29:54Z

test:mpich/ch4/argobots

shintaro-iwasaki · 2020-04-12T03:57:42Z

test:mpich/ch4/argobots
test:mpich/ch3/most
test:mpich/ch4/most

shintaro-iwasaki · 2020-04-12T14:14:09Z

test:mpich/ch4/argobots
test:mpich/ch3/most
test:mpich/ch4/most

shintaro-iwasaki · 2020-04-12T17:16:12Z

test:mpich/ch4/argobots
test:mpich/ch3/most
test:mpich/ch4/most

shintaro-iwasaki · 2020-04-12T20:07:49Z

test:mpich/ch4/argobots

shintaro-iwasaki · 2020-04-12T20:10:21Z

test:mpich/ch4/ofi

shintaro-iwasaki · 2020-04-12T23:34:03Z

Finally all checks passed. @hzhou could you please take a look at this PR when you have time?

hzhou · 2020-04-12T23:58:15Z

Finally all checks passed. @hzhou could you please take a look at this PR when you have time?

Sure, I'll review it tomorrow. Thank you for the effort.

hzhou

Looks good. Just a minor comment. I am approving it. There is no need to rerun the tests if you just amend the comment.

hzhou · 2020-04-13T14:14:40Z

test/mpi/threads/spawn/th_taskmaster.c

@@ -101,6 +101,10 @@ int main(int argc, char *argv[])
 #endif /* USE_THREADS */

 #ifdef USE_THREADS
+        /* Initialize the thread package. It should not be initialized via
+         * MTest_Init_thread since MTest_Finalize cannot be called. */


It is because both MTest_Init_thread and MTest_Finalize are not called rather than cannot be called, right?

You are right. I will change the comment. Thanks a lot for reviewing this PR!

@hzhou I fixed the comment.

th_taskmaster.c uses MTest thread functions for forking and joining threads, but the thread package is not initialized. This patch fixes it. Note that we cannot explicitly call MTest_Init_thread() since the corresponding MTest_Finalize() prints "No Errors" as many as spawned MPI processes, breaking this test.

hzhou · 2020-05-06T03:32:07Z

test/mpi/threads/comm/idup_deadlock.c

@@ -41,6 +41,7 @@ MTEST_THREAD_RETURN_TYPE test_comm_dup(void *arg)
            wait = 0;
            for (i = 0; i < NUM_THREADS; i++)
                wait += start_idup[i];
+            MTest_thread_yield();


@shintaro-iwasaki MTest_thread_yield() is not in a header file that is included in idup_deadlock.c, causing a compiler warning. Could you fix it?

Oops I missed it. #4503 should fix it. Thanks!

It was missed in pmodels#4421.

This patch adds MTest_thread_yield() to tests that I missed in pmodels#4421. Note this does not affect the program behavior if OS-level threads (Pthreads) are used because MTest_thread_yield() is mapped to no-op.

shintaro-iwasaki mentioned this pull request Mar 30, 2020

test/mpich/threads: MTest_init_thread_pkg() is not called. #4422

Closed

shintaro-iwasaki closed this Mar 30, 2020

shintaro-iwasaki reopened this Mar 30, 2020

shintaro-iwasaki requested a review from hzhou March 30, 2020 10:02

hzhou reviewed Mar 30, 2020

View reviewed changes

shintaro-iwasaki force-pushed the FixArgobotsSupport branch from 23fc7c3 to 8d0bd77 Compare March 30, 2020 14:54

hzhou reviewed Mar 30, 2020

View reviewed changes

shintaro-iwasaki mentioned this pull request Apr 3, 2020

test/mpich/threads: fix multithreading tests #4429

Merged

9 tasks

shintaro-iwasaki force-pushed the FixArgobotsSupport branch from 8d0bd77 to a454606 Compare April 8, 2020 15:04

shintaro-iwasaki added 4 commits April 11, 2020 10:28

mpl/thread/argobots: fix MPL_thread_id_t implementation

70b96b5

test/mpi/argobots: use a larger stack size

db30d4f

The default stack size of Argobots threads is too small and can cause a stack overflow bug. This patch fixes it.

shintaro-iwasaki force-pushed the FixArgobotsSupport branch from dc8a0df to 0e18727 Compare April 11, 2020 15:29

shintaro-iwasaki force-pushed the FixArgobotsSupport branch from 0e18727 to a803757 Compare April 12, 2020 03:57

shintaro-iwasaki force-pushed the FixArgobotsSupport branch from a803757 to 440590e Compare April 12, 2020 14:13

shintaro-iwasaki force-pushed the FixArgobotsSupport branch from 440590e to c3561d7 Compare April 12, 2020 17:15

hzhou previously approved these changes Apr 13, 2020

View reviewed changes

shintaro-iwasaki dismissed hzhou’s stale review via f220643 April 13, 2020 15:10

shintaro-iwasaki force-pushed the FixArgobotsSupport branch from c3561d7 to f220643 Compare April 13, 2020 15:10

hzhou approved these changes Apr 13, 2020

View reviewed changes

shintaro-iwasaki merged commit a5865d0 into pmodels:master Apr 13, 2020

hzhou reviewed May 6, 2020

View reviewed changes

shintaro-iwasaki added a commit to shintaro-iwasaki/mpich that referenced this pull request May 6, 2020

mpi/test/threads: add declaration of MTest_thread_yield

4717c34

It was missed in pmodels#4421.

shintaro-iwasaki mentioned this pull request May 6, 2020

mpi/test/threads: add declaration of MTest_thread_yield #4503

Merged

10 tasks

shintaro-iwasaki mentioned this pull request May 15, 2020

test/mpi/threads: fix multithreaded tests with Argobots #3862

Merged

7 tasks

shintaro-iwasaki deleted the FixArgobotsSupport branch October 30, 2020 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Argobots support #4421

Fix Argobots support #4421

shintaro-iwasaki commented Mar 30, 2020 •

edited by hzhou

Loading

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020 •

edited

Loading

hzhou Mar 30, 2020

shintaro-iwasaki Mar 30, 2020

shintaro-iwasaki Apr 12, 2020

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

hzhou Mar 30, 2020

shintaro-iwasaki Mar 30, 2020

hzhou Mar 30, 2020

shintaro-iwasaki Mar 30, 2020

hzhou Mar 30, 2020

shintaro-iwasaki Mar 30, 2020

shintaro-iwasaki Apr 12, 2020

shintaro-iwasaki commented Apr 8, 2020

shintaro-iwasaki commented Apr 8, 2020

shintaro-iwasaki commented Apr 10, 2020

shintaro-iwasaki commented Apr 11, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

hzhou commented Apr 12, 2020

hzhou left a comment

hzhou Apr 13, 2020

shintaro-iwasaki Apr 13, 2020

shintaro-iwasaki Apr 13, 2020

hzhou May 6, 2020

shintaro-iwasaki May 6, 2020

Fix Argobots support #4421

Fix Argobots support #4421

Conversation

shintaro-iwasaki commented Mar 30, 2020 • edited by hzhou Loading

Pull Request Description

Note

Expected Impact

Author Checklist

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

shintaro-iwasaki commented Mar 30, 2020

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

shintaro-iwasaki commented Apr 8, 2020

shintaro-iwasaki commented Apr 8, 2020

shintaro-iwasaki commented Apr 10, 2020

shintaro-iwasaki commented Apr 11, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

shintaro-iwasaki commented Apr 12, 2020

hzhou commented Apr 12, 2020

hzhou 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

Choose a reason for hiding this comment

shintaro-iwasaki commented Mar 30, 2020 •

edited by hzhou

Loading

shintaro-iwasaki commented Mar 30, 2020 •

edited

Loading