Fix threads framework #15
Conversation
|
I'm finding some problems here. Will do a review. |
|
Thanks! |
| @@ -30,3 +30,5 @@ libmca_threads_pthreads_la_SOURCES = \ | |||
| threads_pthreads_tsd.h \ | |||
| threads_pthreads_wait_sync.c \ | |||
| threads_pthreads_wait_sync.h | |||
|
|
|||
| AM_LDFLAGS = -labt | |||
There was a problem hiding this comment.
I don't think this is right. Why add -labt for pthreads specific Makefile.am?
There was a problem hiding this comment.
You are right. It is wrong. I will fix it soon.
There was a problem hiding this comment.
I wrongly put it in pthreads/Makefile.am. This AM_LDFLAGS was moved to argobots/Makefile.am.
| @@ -115,8 +115,6 @@ OPAL_DECLSPEC OBJ_CLASS_DECLARATION(opal_recursive_mutex_t); | |||
| } | |||
| #endif | |||
|
|
|||
| #endif | |||
There was a problem hiding this comment.
why did you remove this endif? it looks correct to me. It closes the defined(OPAL_PTHREAD_RECURSIVE_MUTEX_INITIALIZER) no?
There was a problem hiding this comment.
This change should have been done to qthreads/threads_qthreads_mutex.h instead. Fixed.
| @@ -30,7 +30,7 @@ | |||
|
|
|||
| static inline void opal_threads_argobots_ensure_init(void) | |||
| { | |||
| if (ABT_initialized() != 0) { | |||
| if (ABT_initialized() != ABT_SUCCESS) { | |||
There was a problem hiding this comment.
would you mind reversing the check to have ABT_SUCCESS != ABT_initialize()? I know it looks weird but that's the "openmpi" coding style.
There was a problem hiding this comment.
Thank you. I will follow the Open MPI style. I will examine other checks.
There was a problem hiding this comment.
I found some did not follow.
if (NULL == t->t_run || t->t_handle 1= (pthread_t)-1) {
to
if (NULL == t->t_run || (pthread_t)-1 != t->t_handle) {
I fixed them as far as I could find.
Signed-off-by: Shintaro Iwasaki <siwasaki@anl.gov>
|
I fixed them. Could you please look at this PR again? |
* piggybacking Bull functionalities * coll/adapt: Fix naming conventions and C11 atomic use This commit fixes some naming convention issues, such as function names which should follow the naming ompi_coll_adapt instead of mca_coll_adapt, reserved for component and module naming (cf. tuned collective component); It also fixes the use of _Atomic construct, which is only valid in C11. OPAL constructs have already been adapted to that use, so use opal_atomic_* types instead. * coll/adapt: Remove unused component field in module This commit removes an unneeded field referencing the component in the module of adapt, as it is already available through the mca_coll_adapt_component global variable. Signed-off-by: Marc Sergent <marc.sergent@atos.net> Co-authored-by: Lemarinier, Pierre <pierre.lemarinier@atos.net> Co-authored-by: pierrele <31764860+pierrele@users.noreply.github.com>
This PR addresses most of the problems newly found except the following items that I think are still discussed:
REQUEST_PENDINGandREQUEST_COMPLETEDThe head repository is
Could you please squash this branch so that I can resolve some of his comments.