Skip to content

Improve request/progress performance #1810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 22, 2016
Merged

Improve request/progress performance #1810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
143a93f
opal/sync: remove usage of OPAL_ENABLE_MULTI_THREADS
hjelmn Jun 22, 2016
e4f920f
opal/progress: improve performance when there are no LP callbacks
hjelmn Jun 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ompi/request/request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,8 @@ static inline int ompi_request_complete(ompi_request_t* request, bool with_signa

if( OPAL_LIKELY(with_signal) ) {
if(!OPAL_ATOMIC_CMPSET_PTR(&request->req_complete, REQUEST_PENDING, REQUEST_COMPLETED)) {
ompi_wait_sync_t *tmp_sync = (ompi_wait_sync_t *) OPAL_ATOMIC_SWP_PTR(&request->req_complete,
REQUEST_COMPLETED);
ompi_wait_sync_t *tmp_sync = (ompi_wait_sync_t *) OPAL_ATOMIC_SWAP_PTR(&request->req_complete,
REQUEST_COMPLETED);
/* In the case where another thread concurrently changed the request to REQUEST_PENDING */
if( REQUEST_PENDING != tmp_sync )
wait_sync_update(tmp_sync, 1, request->req_status.MPI_ERROR);
Expand Down
2 changes: 1 addition & 1 deletion opal/runtime/opal_progress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ opal_progress(void)
events += (callbacks[i])();
}

if ((OPAL_THREAD_ADD32((volatile int32_t *) &num_calls, 1) & 0x7) == 0) {
if (callbacks_lp_len > 0 && (OPAL_THREAD_ADD32((volatile int32_t *) &num_calls, 1) & 0x7) == 0) {
/* run low priority callbacks once every 8 calls to opal_progress() */
for (i = 0 ; i < callbacks_lp_len ; ++i) {
events += (callbacks_lp[i])();
Expand Down
14 changes: 14 additions & 0 deletions opal/threads/mutex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,20 @@ OPAL_THREAD_ADD_SIZE_T(volatile size_t *addr, int delta)
#endif


static inline void *opal_thread_swap_ptr (volatile void *ptr, void *newvalue)
{
if (opal_using_threads ()) {
return opal_atomic_swap_ptr (ptr, newvalue);
}

void *old = ((void **) ptr)[0];
((void **) ptr)[0] = newvalue;

return old;
}

#define OPAL_ATOMIC_SWAP_PTR(x, y) opal_thread_swap_ptr (x, y)

END_C_DECLS

#endif /* OPAL_MUTEX_H */
11 changes: 2 additions & 9 deletions opal/threads/wait_sync.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
* Copyright (c) 2014-2016 The University of Tennessee and The University
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2016 Los Alamos National Security, LLC. All rights
* reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand All @@ -21,15 +23,6 @@ static ompi_wait_sync_t* wait_sync_list = NULL;
pthread_mutex_unlock( &(who)->lock); \
} while(0)


int sync_wait_st(ompi_wait_sync_t *sync)
{
while(sync->count > 0) {
opal_progress();
}
return (0 == sync->status) ? OPAL_SUCCESS : OPAL_ERROR;
}

int sync_wait_mt(ompi_wait_sync_t *sync)
{
if(sync->count <= 0)
Expand Down
65 changes: 31 additions & 34 deletions opal/threads/wait_sync.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
* Copyright (c) 2014-2016 The University of Tennessee and The University
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2016 Los Alamos National Security, LLC. All rights
* reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand All @@ -27,50 +29,44 @@ typedef struct ompi_wait_sync_t {
#define REQUEST_PENDING (void*)0L
#define REQUEST_COMPLETED (void*)1L

#if OPAL_ENABLE_MULTI_THREADS

#define OPAL_ATOMIC_ADD_32(a,b) opal_atomic_add_32(a,b)
#define OPAL_ATOMIC_SWP_PTR(a,b) opal_atomic_swap_ptr(a,b)
#define SYNC_WAIT(sync) sync_wait_mt(sync)
#define PTHREAD_COND_INIT(a,b) pthread_cond_init(a,b)
#define PTHREAD_MUTEX_INIT(a,b) pthread_mutex_init(a,b)
#define SYNC_WAIT(sync) (opal_using_threads() ? sync_wait_mt (sync) : sync_wait_st (sync))
#define PTHREAD_COND_INIT(a,b) (opal_using_threads() ? pthread_cond_init (a,b) : 0)
#define PTHREAD_MUTEX_INIT(a,b) (opal_using_threads() ? pthread_mutex_init (a,b) : 0)

#define WAIT_SYNC_RELEASE(sync) \
do { \
if (opal_using_threads()) { \
pthread_cond_destroy(&(sync)->condition); \
pthread_mutex_destroy(&(sync)->lock); \
} while(0)
}

#define WAIT_SYNC_SIGNAL(sync) \
do { \
if (opal_using_threads()) { \
pthread_mutex_lock(&(sync->lock)); \
pthread_cond_signal(&sync->condition); \
pthread_mutex_unlock(&(sync->lock)); \
} while(0)

#else
}

#define OPAL_ATOMIC_ADD_32(a,b) (*(a) += (b))
#define OPAL_ATOMIC_SWP_PTR(a,b) *(a) = (b)
#define PTHREAD_COND_INIT(a,b)
#define PTHREAD_MUTEX_INIT(a,b)
#define SYNC_WAIT(sync) sync_wait_st(sync)
#define WAIT_SYNC_RELEASE(sync)
#define WAIT_SYNC_SIGNAL(sync)
OPAL_DECLSPEC int sync_wait_mt(ompi_wait_sync_t *sync);
static inline int sync_wait_st (ompi_wait_sync_t *sync)
{
while (sync->count > 0) {
opal_progress();
}

#endif /* OPAL_ENABLE_MULTI_THREADS */
return sync->status;
}

OPAL_DECLSPEC int sync_wait_mt(ompi_wait_sync_t *sync);
OPAL_DECLSPEC int sync_wait_st(ompi_wait_sync_t *sync);

#define WAIT_SYNC_INIT(sync,c) \
do { \
(sync)->count = c; \
(sync)->next = NULL; \
(sync)->prev = NULL; \
(sync)->status = 0; \
PTHREAD_COND_INIT(&(sync)->condition, NULL); \
PTHREAD_MUTEX_INIT(&(sync)->lock, NULL); \
#define WAIT_SYNC_INIT(sync,c) \
do { \
(sync)->count = c; \
(sync)->next = NULL; \
(sync)->prev = NULL; \
(sync)->status = 0; \
if (opal_using_threads()) { \
Copy link
Member

@nysal nysal Jun 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since PTHREAD_COND_INIT and PTHREAD_MUTEX_INIT already check for opal_using_threads(), this branch might be redundant ?

Copy link
Member Author

@hjelmn hjelmn Jun 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I didn't think to check what those macro did. Will clean it up in a bit.

Copy link
Member Author

@hjelmn hjelmn Jun 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you want to do it. I can add your commit to the 2.x PR.

Copy link
Member

@nysal nysal Jun 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see your comment until later, timezone issues. I see that you already have a PR to fix it - #1811

Thanks 👍

PTHREAD_COND_INIT(&(sync)->condition, NULL); \
PTHREAD_MUTEX_INIT(&(sync)->lock, NULL); \
} \
} while(0)

/**
Expand All @@ -82,12 +78,13 @@ OPAL_DECLSPEC int sync_wait_st(ompi_wait_sync_t *sync);
static inline void wait_sync_update(ompi_wait_sync_t *sync, int updates, int status)
{
if( OPAL_LIKELY(OPAL_SUCCESS == status) ) {
if( 0 != (OPAL_ATOMIC_ADD_32(&sync->count, -updates)) ) {
if( 0 != (OPAL_THREAD_ADD32(&sync->count, -updates)) ) {
return;
}
} else {
OPAL_ATOMIC_CMPSET_32(&(sync->count), 0, 0);
sync->status = -1;
/* this is an error path so just use the atomic */
Copy link
Member Author

@hjelmn hjelmn Jun 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want to point out this error path was wrong. OPAL_ATOMIC_CMPSET_32(&(sync->count), 0, 0); does nothing.

opal_atomic_swap_32 (&sync->count, 0);
sync->status = OPAL_ERROR;
}
WAIT_SYNC_SIGNAL(sync);
}
Expand Down