prov/shm: Add unmap_region function #10364
Conversation
|
bot:aws:retest |
zachdworkin
force-pushed
the
ze-shm
branch
3 times, most recently
from
ad41c78
to
5c3e25b
Compare
| node = ofi_rbmap_find(&smr_av->smr_map.rbmap, | ||
| &smr_av->smr_map.peers[shm_id].peer.name); | ||
| assert(node); | ||
| ret = smr_map_del(&smr_av->smr_map, node); |
There was a problem hiding this comment.
Now that both places where you call smr_map_del do a find and pass in the node, let's have the call take in the id instead and do the find in there. You can use the rbmap_find_delete call too to simplify.
This function is mainly for the niche case where on progress_connreq a peer is added to the map with its region needing to be mapped, and then after mapping it, it's discovered that the newly mapped peer's process died. In this case we need to unmap them and free any resources that were opened for communicating with them. With the creation of this function we can rework smr_map_del to use it as common code. This requires changes to smr_av.c where smr_map_del is called. smr_map_del is now an iterable function. This is to optimize smr_map_cleanup to use ofi_rbmap_foreach to only cleanup peers that exist. Remove lock from map_to_region and unmap_region functions and require lock acquirement before calling those functions. This is necessary because on av removal path map will be double locked if the functions also process locking the map. The map_to_region function is updated to mirror this policy. Signed-off-by: Zach Dworkin <zachary.dworkin@intel.com>
|
@shijin-aws whats the AWS failure? |
|
Lots of failures for Open MPI RMA test, one example is |
|
I can try reproducing locally to get more logs. But it seems to me the segfaults happened in some locking steps |
I can test it locally. We dont test OMPI in our CI so those tests weren't run on our end. |
|
Does any provider other than efa can access shm inside Open MPI run now? Open MPI cannot run with shm itself as it doesn't support FI_REMOTE_COMM |
|
I have a "hack" for OMPI to force it to run shm without needing another provider. I just remove taht dependency and then run tests that dont need it. Im pretty sure this bug is all related so fixing one of those tests will likely fix most/all of them |
|
Thanks. The interesting thing is that the error only happens for our inter-node test, in this case we only touch shm during the control interface operations (av_insert, ep open, mr reg), no data transfer |
|
do you have a stack trace with the av_insert path? I removed the lock inside of map_to_region and pushed it to be outside. I only moved the locks in prov/shm. I think I need to take a look at where it is called in prov/efa. Thats likely the bug |
|
The bug is triggered at the MPI_Finalize stage, I think it's in the clean up stage |
|
I wonder if there is a path we dont test in Intel CI that removes nodes from the rb tree after I already remove them with the updated call. Ill take a look there |
|
@shijin-aws I have been unsuccessful in recreating the same bug locally. Do you know which call is causing the segfault? I assume its in smr_ep_close. I also found a missed lock in smr_ep.c:227 needs to be locked and unlocked afterwards |
This function is mainly for the niche case where on progress_connreq a peer is added to the map with its region needing to be mapped, and then after mapping it, it's discovered that the newly mapped peer's process died. In this case we need to unmap them and free any resources that were opened for communicating with them.
With the creation of this function we can rework smr_map_del to use it as common code. This requires changes to smr_av.c where smr_map_del is called. smr_map_del is now an iterable function. This is to optimize smr_map_cleanup to use ofi_rbmap_foreach to only cleanup peers that exist.