OSHMEM/MCA/SPML/UCX: v1.5 implement put_signal and put_signal_nbi #13568
Conversation
roiedanino
changed the title
oshmem/mca/spml/ucx/spml_ucx.c
Outdated
| return res; | ||
| } | ||
|
|
||
| return mca_spml_ucx_signal_common(ctx, sig_addr, signal, sig_op, dst); |
There was a problem hiding this comment.
Don't we need a fence here?
There was a problem hiding this comment.
Not according to the v1.5 spec
There was a problem hiding this comment.
Not according to the v1.5 spec
can you pls elaborate why not needed?
There was a problem hiding this comment.
Also, should we use non-blocking signal here?
There was a problem hiding this comment.
From v1.5 documentation, unless I missed something:
There was a problem hiding this comment.
My bad, we do need a fence here
oshmem/mca/spml/ucx/spml_ucx.c
Outdated
| return OSHMEM_ERROR; | ||
| } | ||
|
|
||
| return res; |
There was a problem hiding this comment.
maybe move the return to lines 1662/1658 directly?
oshmem/mca/spml/ucx/spml_ucx.c
Outdated
| return res; | ||
| } | ||
|
|
||
| return mca_spml_ucx_signal_common(ctx, sig_addr, signal, sig_op, dst); |
There was a problem hiding this comment.
need fence before this, to ensure ordering
oshmem/mca/spml/ucx/spml_ucx.c
Outdated
| return res; | ||
| } | ||
|
|
||
| return mca_spml_ucx_signal_common(ctx, sig_addr, signal, sig_op, dst); |
There was a problem hiding this comment.
Not according to the v1.5 spec
can you pls elaborate why not needed?
oshmem/mca/spml/ucx/spml_ucx.c
Outdated
| return res; | ||
| } | ||
|
|
||
| return mca_spml_ucx_signal_common(ctx, sig_addr, signal, sig_op, dst); |
There was a problem hiding this comment.
Also, should we use non-blocking signal here?
|
@yosefe Can re-review please? |
oshmem/mca/spml/ucx/spml_ucx.c
Outdated
| } | ||
|
|
||
| /* This routine is not implemented */ | ||
| static inline int mca_spml_ucx_signal_common(shmem_ctx_t ctx, |
There was a problem hiding this comment.
split to mca_spml_ucx_signal_common and mca_spml_ucx_signal_common_nb
roiedanino
force-pushed
the
oshmem/put-signal
branch
2 times, most recently
from
006d7f1 to
963553a
Compare
oshmem/mca/spml/ucx/spml_ucx.c
Outdated
| uint64_t *sig_addr, | ||
| uint64_t signal, | ||
| int sig_op, | ||
| int dst) |
roiedanino
force-pushed
the
oshmem/put-signal
branch
from
cba77c9 to
26fa31a
Compare
yosefe
left a comment
yosefe
left a comment
There was a problem hiding this comment.
it seems my previous comment about using non-blocking atomic was wrong. "add" and "swap" a
| int pe) | ||
| { | ||
| #if HAVE_DECL_UCP_ATOMIC_OP_NBX | ||
| return mca_atomic_ucx_op_nb(ctx, target, value, size, pe, UCP_ATOMIC_OP_SWAP); |
There was a problem hiding this comment.
how mca_atomic_ucx_op_nb is different from mca_atomic_ucx_op?
can't we just reuse mca_atomic_ucx_op?
| return res; | ||
| } | ||
|
|
||
| return mca_spml_ucx_signal(ctx, sig_addr, signal, sig_op, dst); |
There was a problem hiding this comment.
we need fence also here in the blocking version because it controls the remote ordering
Signed-off-by: Roie Danino <[email protected]> OSHMEM/MCA/ATOMIC: added atomic_set mca function, and made non-retrieving atomic operations non-blocking Signed-off-by: Roie Danino <[email protected]>
roiedanino
force-pushed
the
oshmem/put-signal
branch
from
6a1e665 to
1afefcf
Compare
4 participants
What?
Implements
mca_spml_ucx_put_signalandmca_spml_ucx_put_signal_nbfor OpenSHMEM signaling operations with UCX backend.Why
The put_signal operations are part of the OpenSHMEM 1.5 specification and were previously unimplemented (returning OSHMEM_ERR_NOT_IMPLEMENTED).