Conversation
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
devreal
left a comment
devreal
left a comment
There was a problem hiding this comment.
Looks good, just a few comments.
| + (uint64_t)notify * sizeof(uint64_t); | ||
| } | ||
|
|
||
| #define CHECK_NOTIFY_IDX(notify) \ |
There was a problem hiding this comment.
I would suggest passing in the module and checking against a value stored in there. OMPI_OSC_UCX_MAX_NOTIFY_COUNTERS is just a crutch until we have a better solution
|
|
||
| /* Counters are local memory — just read with a barrier to ensure | ||
| * any preceding remote writes to this counter are visible. */ | ||
| opal_atomic_rmb(); |
There was a problem hiding this comment.
Not sure this actually does anything. Shouldn't the read barrier come after the counter read to prevent subsequent reads from being reordered?
|
|
||
| volatile uint64_t *counter = | ||
| (volatile uint64_t *)(module->addrs[my_rank] + module->size) + notify; | ||
| *value = (OMPI_MPI_COUNT_TYPE)opal_atomic_swap_64((volatile int64_t *)counter, 0); |
There was a problem hiding this comment.
You will need to use UCX to swap the value, otherwise it is not guaranteed to be atomic wrt to other network atomic operations.
| /* create the segment */ | ||
|
|
||
| size_t total = 0; | ||
| size_t notify_size = OMPI_OSC_UCX_MAX_NOTIFY_COUNTERS * sizeof(uint64_t); |
There was a problem hiding this comment.
Replace the use of OMPI_OSC_UCX_MAX_NOTIFY_COUNTERS with a local variable that we can set to something that comes out of the info object. That makes it easier going forward. Also, store the value of that variable in the module (see my earlier comment).
|
Oh, I guess we'll need to get #9 in first? |
2 participants
Signed-off-by: Joseph Antony jajoseph.antony18@gmail.com