opal/mca/threads: remove code duplication #8790
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can one of the admins verify this patch? |
|
I'd truly appreciate it if @devreal could review this PR, too. Of course, I appreciate any comments from anyone. |
|
ok to test |
abouteiller
requested changes
Apr 12, 2021
shintaro-iwasaki
force-pushed
the
topic/thread_remove_dup
branch
from
d7ed306 to
0745b45
Compare
devreal
requested changes
Apr 12, 2021
shintaro-iwasaki
force-pushed
the
topic/thread_remove_dup
branch
from
0745b45 to
a048664
Compare
devreal
reviewed
Apr 13, 2021
shintaro-iwasaki
force-pushed
the
topic/thread_remove_dup
branch
from
a048664 to
4e48be9
Compare
devreal
approved these changes
Apr 13, 2021
bosilca
reviewed
Apr 13, 2021
bosilca
reviewed
Apr 13, 2021
bosilca
reviewed
Apr 13, 2021
shintaro-iwasaki
force-pushed
the
topic/thread_remove_dup
branch
3 times, most recently
from
f90481b to
fea82f5
Compare
abouteiller
approved these changes
Apr 14, 2021
|
Thank you for reviewing this PR. @bosilca Could you please review this PR when you have time? |
bosilca
reviewed
Apr 14, 2021
bosilca
approved these changes
Apr 15, 2021
|
Could one of the admins please merge this PR? This PR is necessary for compilation of Open MPI + Qthreads (@janciesko). If rebase is needed, please let me know. |
|
@shintaro-iwasaki please squash and then we can merge. |
opal_pthread_mutex_t is not used. This patch removes it. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/qthreads,argobots: remove m_recursive m_recursive is not used. This patch removes it. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/qthreads,argobots: update wait_sync implementation This patch is corresponding to changes to the following commit for Pthreads: 6a406fb Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/pthreads: implement opal_thread_internal_mutex/cond_t opal_thread_internal_mutex_t and opal_thread_internal_cond_t are new objects that are directly mapped to raw mutex/condition variable implementations (e.g., pthread_mutex_t for Pthreads). This abstraction is different from opal_mutex_t, which has more functionalities. Introducing this abstraction helps removal of code duplication among other threading layers. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/pthreads: use opal_thread_internal_mutex/cond_t opal_thread_internal_mutex/cond routines are static inline functions, so there should be no additional overhead. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/argobots: implement opal_thread_internal_mutex/cond_t Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/argobots: use opal_thread_internal_mutex/cond_t Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/qthreads: implement opal_thread_internal_mutex/cond_t Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads/qthreads: use opal_thread_internal_mutex/cond_t Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads: move mutex implementation to threads/base Now implementations of opal_mutex_xxx() and opal_cond_xxx() are the same. This patch moves them to base/mutex.c and mutex.h and removes the duplication. Note that those implementations are copied from pthread/threads_pthreads_mutex.c and pthread/threads_pthreads_mutex.h. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads: use opal_thread_yield() in busy loop if requested This commit is prerequisite for the next commit that unifies the implementation of wait_sync. Nonpreemptive threads like Qthreads and Argobots need a yield in a busy loop to avoid a deadlock, while Pthreads does not. To unify the implementation in the next commit, this patch adds opal_thread_yield() if opal_progress_yield_when_idle is true. At the same time, this patch removes a yield operation after opal_progress() since opal_progress() internally yields when it does not make any progress. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads: move wait_sync implementation to threads/base Now implementations of wait_sync_xxx() functions and macros are the same. This patch moves them to base/wait_sync.c and wait_sync.h and removes the duplication. Note that those implementations are copied from pthread/threads_pthreads_wait_sync.c and pthread/threads_pthreads_wait_sync.h. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads: change Pthread-specific names This patch renames variables and functions that are no longer specific to Pthread. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads: remove MCA_threads_wait_sync_base_include_xxx MCA_threads_wait_sync_base_include_xxx is no longer used. This patch removes them. Signed-off-by: Shintaro Iwasaki <[email protected]> opal/mca/threads: minor cleanup - Remove unnecessary "include" - Remove weird empty lines - Clean up parentheses for macros - Remove EDEADLK check for trylock() and unlock() Signed-off-by: Shintaro Iwasaki <[email protected]>
shintaro-iwasaki
force-pushed
the
topic/thread_remove_dup
branch
from
fea82f5 to
bcd376a
Compare
|
Thanks. I squashed all. If there is any formatting rules regarding "squash", please let me know. |
|
There are no rules, it just just practice to have a clean history and avoid PR with many commits where each one only fixes a minor issue. |
|
Actually, I looked at the series of commits before squashing -- I thought they were all nice, small, tidy commits. I would have been happy with them without squashing. Makes the history easier to understand. So ya -- there's no rules. Just opinions. 😄 |
|
I might be a little bit more picky on this. When I see different commits with the same log message, I tend to assume they should be squashed. |
|
Could someone merge this PR? This PR is necessary for compilation of Open MPI + Qthreads. If I need to rebase/squash/ask additional reviews, please let me know. If other PRs are blocking this, I'd appreciate it if someone could teach me when this PR can be merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Pthreads, Qthreads, and Argobots share a lot of logics in common. This duplication hinders code maintenance.
(Specifically, Qthreads and Argobots code bases need to be updated frequently.)
This PR is what I promised in #8767. I'd like to thank @devreal for his suggestion.
Solution
Both Qthreads and Argobots have implementations that are similar to
pthread_mutex_tandpthread_cond_t. If those raw mutex/cond objects are abstracted, a lot of duplicated code can be removed. This PR introducesopal_thread_internal_mutex_tandopal_thread_internal_cond_tto remove code duplication inmutex.h,mutex.c,wait_sync.h, andwait_sync.c.Since this layer is very thin (
static inlinefunctions), this abstraction should not introduce an additional overhead.I have checked Pthreads, Qthreads, and Argobots versions locally (not very thoroughly, though). I believe the Pthreads configuration will be tested by CI, so my point is that this change worked for Qthreads and Argobots as far as I checked.
Structure of this PR
Commits 1 - 3
Clean up each logic to unify functions later. These commits are either fixing bugs or removing something unnecessary.
Commits 4 - 10
Introduce
opal_thread_internal_mutex_tandopal_thread_internal_cond_tand use them for each thread (Pthreads, Qthreads, Argobots).The first commit implements
opal_thread_internal_mutex_tandopal_thread_internal_cond_tand the second commit applies them.Those commits do not change the logic; it just wraps
pthread_mutex_xxx()andpthread_cond_xxx()byopal_thread_internal_mutex_xxx()andopal_thread_internal_cond_xxx().Commit 11
Implement common mutex-related logics in
base/mutex.candmutex.hand remove the corresponding code in each threading directory.All the macros and functions are copied from
pthread/threads_pthreads_mutex.candpthread/threads_pthreads_mutex.h.Commits 12 - 13
Implement common
wait_sync-related logics inbase/wait_sync.candwait_sync.hand remove the corresponding code in each threading directory.All the macros and functions are copied from
pthread/threads_pthreads_wait_sync.candpthread/threads_pthreads_wait_sync.h.Commits 14 - 16
Clean up opal/mca/threads code.