Optimized put with signal implementation as shmemx routines #815
Conversation
Signed-off-by: Md <[email protected]>
…implementation for portals4 Signed-off-by: Md <[email protected]>
Signed-off-by: Md <[email protected]>
…ompletion argument Signed-off-by: Md <[email protected]>
Signed-off-by: Md <[email protected]>
Signed-off-by: Md <[email protected]>
Signed-off-by: Md <[email protected]>
|
@jdinan @davidozog Ready for re-review. |
| shmem_transport_fence(ctx); | ||
| shmem_transport_put_scalar(ctx, sig_addr, &signal, sizeof(uint64_t), pe); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you add a "FIXME" comment and/or file an issue to go back and optimize nonblocking put-with-signal in the Portals transport?
| if (len == 0) { | ||
| shmem_internal_put_scalar(ctx, sig_addr, &signal, sizeof(uint64_t), pe); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Code looks good. Just wanted to suggest double checking the proposal that this is the right way to handle the len == 0 case.
There was a problem hiding this comment.
There is no specific text added in the proposal for this case. However, in the discussion, it is mentioned that sending the signal is the expected behavior.
src/transport_ofi.h
Outdated
| }; | ||
|
|
||
| do { | ||
| ret = fi_writemsg(ctx->ep, &msg, FI_DELIVERY_COMPLETE); |
There was a problem hiding this comment.
Do you mean to add the FI_INJECT flag here (this is the buffered send case)?
There was a problem hiding this comment.
Yes, FI_INJECT should be added here. Thanks.
|
|
||
| frag_source += frag_len; | ||
| frag_target += frag_len; | ||
| } |
There was a problem hiding this comment.
Just making an observation that when fragmentation occurs, the signal will be signaling completion of multiple prior puts. I re-read the FI_FENCE text and I think this is ok. It might not hurt to toss a comment somewhere to the effect that multiple prior operations may be fenced by the signal put when fragmentation occurs.
| }; | ||
|
|
||
| do { | ||
| ret = fi_atomicmsg(ctx->ep, &msg_signal, FI_DELIVERY_COMPLETE | FI_FENCE | FI_INJECT); |
There was a problem hiding this comment.
The open question is, whether we also need to use an atomic to implement shmem_p. Could you add a comment here and open an issue so we remember to revisit this?
…ed send Signed-off-by: Md <[email protected]>
No description provided.