-
Notifications
You must be signed in to change notification settings - Fork 0
modifying osc mt code #2
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.
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
base: master
Are you sure you want to change the base?
Conversation
ompi/mca/osc/ucx/osc_ucx_comm.c
Outdated
| return ret; | ||
| } | ||
|
|
||
| if (pthread_equal(tid, mca_osc_ucx_component.main_tid)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that main_tid works with "receve" worker?
ompi/mca/osc/ucx/osc_ucx_comm.c
Outdated
| return ret; | ||
| } | ||
|
|
||
| if (pthread_equal(tid, mca_osc_ucx_component.main_tid)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth making this piece an inline function so it doesn't duplicate in put & get
ompi/mca/osc/ucx/osc_ucx_comm.c
Outdated
| } | ||
| } | ||
|
|
||
| curr_thread_info = pthread_getspecific(my_thread_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would shift it in the "if" statement.
Otherwise you are always calling pthread_getspecific twice which we don't want
ompi/mca/osc/ucx/osc_ucx_component.c
Outdated
| UCP_PARAM_FIELD_REQUEST_SIZE; | ||
| context_params.features = UCP_FEATURE_RMA | UCP_FEATURE_AMO32 | UCP_FEATURE_AMO64; | ||
| context_params.mt_workers_shared = 0; | ||
| context_params.mt_workers_shared = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought in our implementations workers are not shared, only context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is related to the context. But I wanted to double check.
ompi/mca/osc/ucx/osc_ucx_component.c
Outdated
| if (mca_osc_ucx_component.mem_addr_disps == NULL) | ||
| mca_osc_ucx_component.mem_addr_disps = malloc(comm_size * sizeof(int)); | ||
|
|
||
| if (!is_eps_ready) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case is_ep_ready can be "true"?
| } | ||
| pthread_mutex_unlock(&curr_worker->lock); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to also update ompi_osc_ucx_flush_all?
cf66f98 to
34153d3
Compare
change datastruct
Add Operation function
Osc/mt v2
Intermediate fixes
Bring everything to a "builds OK" state
OSC builds as well.
No description provided.