UCP: Copy memh flags from parent to child #9891
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brminich
previously approved these changes
May 21, 2024
|
@iyastreb can we add a unit test for it? |
Sure, good idea indeed! |
iyastreb
force-pushed
the
ucp/copy-memh-flags-parent
branch
2 times, most recently
from
41f7582
to
b7b605d
Compare
brminich
reviewed
May 23, 2024
src/ucp/core/ucp_mm.c
Outdated
| @@ -1744,6 +1745,7 @@ ucp_memh_import_slow(ucp_context_h context, ucs_rcache_t *existing_rcache, | |||
| user_memh->parent = memh; | |||
| } else { | |||
| memh = user_memh; | |||
| memh->flags |= UCP_MEMH_FLAG_IMPORTED; | |||
There was a problem hiding this comment.
you can set it once for both cases outside of if..else on line 1752 (and remove on line 1744)
test/gtest/ucp/test_ucp_mmap.cc
Outdated
| } | ||
|
|
||
| void test_ucp_mmap::test_rereg_imported_mem(ucp_mem_h memh, | ||
| uint64_t memh_pack_flags, |
There was a problem hiding this comment.
seems memh_pack_flags is not really needed
test/gtest/ucp/test_ucp_mmap.cc
Outdated
| uint64_t memh_pack_flags, | ||
| size_t size) | ||
| ucp_mem_h test_ucp_mmap::import_memh(ucp_mem_h exported_memh, | ||
| uint64_t memh_pack_flags) |
There was a problem hiding this comment.
seems this method is always packign exported key, so memh_pack_flags is not really nedeed
There was a problem hiding this comment.
ok, true, I'll remove this param from this function and 2 others
yosefe
reviewed
May 26, 2024
test/gtest/ucp/test_ucp_mmap.cc
Outdated
| ~mem_chunk(); | ||
| ucp_rkey_h unpack(ucp_ep_h, ucp_md_map_t md_map = 0); | ||
| }; | ||
| struct mem_chunk { |
There was a problem hiding this comment.
maybe move mem_chunk to test_ucp_mmap, so it will not be in top-level namespace?
test/gtest/ucp/test_ucp_mmap.cc
Outdated
| EXPECT_FALSE(mem.memh->flags & UCP_MEMH_FLAG_IMPORTED); | ||
|
|
||
| ucp_mem_h imported_memh = import_memh(mem.memh); | ||
| EXPECT_TRUE(!!(imported_memh->flags & UCP_MEMH_FLAG_IMPORTED)); |
There was a problem hiding this comment.
!! probably not needed here
yosefe
approved these changes
May 27, 2024
iyastreb
force-pushed
the
ucp/copy-memh-flags-parent
branch
from
4415e56
to
2e6e8f7
Compare
brminich
approved these changes
May 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
UCP_MEMH_FLAG_IMPORTEDwas added in #8584, as the only memory handle flag. But this flag is only set in the memh parent, and never propagated to user memh. Despite that, flag is used on user memh:I think this flag must copied from parent to child in
ucp_memh_init_from_parent, so that we can distinguish whether user memh is attached to imported memory or not