ompi/oshmem: fix bug in shmem_alltoall in mca/scoll/basic. #4984
Conversation
|
bot:ompi:retest |
|
@alex-mikheev Please take a look. |
|
@gpaulsen FYI. |
|
@xinzhao3 I pulled down your changes and the test case that produced the original issue is running smoothly! |
|
@sam6258 great! Thanks. |
There was a problem hiding this comment.
Good catch! Please also add a test to the oshmem verifier at https://github.com/openshmem-org/tests-mellanox
|
|
||
| dst_pe = get_dst_pe(group, src_blk_idx, dst_blk_idx); | ||
| dst_pe = get_dst_pe(group, i, dst_blk_idx); | ||
| src_blk_idx = oshmem_proc_group_find_id(group, dst_pe); |
There was a problem hiding this comment.
Calling oshmem_proc_group_find_id should avoided as it does a linear lookup.
the index should be (dst_blk_idx + src_blk_idx) % group->proc_count and it is already computed in get_dst_pe()
xinzhao3
force-pushed
the
topic/shmem-alltoall
branch
from
c186a84 to
3ce5fb0
Compare
|
@alex-mikheev I re-pushed, deleting oshmem_proc_group_find_id(). @sam6258 could you test the branch again? |
| for (i = 0; i < group->proc_count; i++) { | ||
|
|
||
| dst_pe = get_dst_pe(group, src_blk_idx, dst_blk_idx); | ||
| dst_pe = get_dst_pe(group, i, dst_blk_idx); |
There was a problem hiding this comment.
I think it is not correct now. I suggest modifying get_dst_pe so that you get back (dst_pe, dst_pe_index) where
dst_pe_index = (dst_blk_idx + src_blk_idx) % group->proc_count;
dst_pe = oshmem_proc_pe(group->proc_array[dst_pe_index])
Signed-off-by: Xin Zhao <xinz@mellanox.com>
xinzhao3
force-pushed
the
topic/shmem-alltoall
branch
from
3ce5fb0 to
af32c30
Compare
|
@alex-mikheev I modified get_dst_pe(), could you review again? |
|
@xinzhao3 My test still passes. Thanks! |
|
@xinzhao3 please open a PR against 3.0.x and 3.1.x branches. |
|
@jladd-mlnx will do today asap. |
6 participants
This PR fixes the bug in alltoall algorithm in oshmem/mca/scoll/basic. When it calculates dst PE idx, it does index permutation to achieve better distribution of traffic (get_dst_pe() function), therefore, the src block index should be re-calculated from that new dst PE index, not starting from 0.
Signed-off-by: Xin Zhao xinz@mellanox.com