Skip to content

Add support for ucc_mem_map and ucc_mem_unmap #1070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Add support for ucc_mem_map and ucc_mem_unmap #1070

wants to merge 29 commits into from

Conversation

wfaderhold21
Copy link
Collaborator

What

Implementation of PR #1037

NOTE: branch is branched from #1037 and will need to be rebased before merging

@swx-jenkins3
Copy link

Can one of the admins verify this patch?

@wfaderhold21 wfaderhold21 force-pushed the topic/explicit-mem-map-impl branch from 61ab0da to 4a2f9c6 Compare January 16, 2025 00:31
@manjugv
Copy link
Contributor

manjugv commented Jan 18, 2025

@janjust @nsarka Can you test on your setup? Ask @wfaderhold21 for details.

@wfaderhold21 wfaderhold21 force-pushed the topic/explicit-mem-map-impl branch from f02340e to 41f0b4e Compare February 19, 2025 19:54
@wfaderhold21 wfaderhold21 force-pushed the topic/explicit-mem-map-impl branch from 1a327f1 to ebc57c5 Compare February 20, 2025 00:15
@wfaderhold21
Copy link
Collaborator Author

Example user usage of API changes: https://github.com/wfaderhold21/simple-ucc-ex

wfaderhold21
wfaderhold21 commented Feb 20, 2025
View reviewed changes
wfaderhold21
wfaderhold21 commented Feb 20, 2025
View reviewed changes
wfaderhold21
wfaderhold21 commented Feb 20, 2025
View reviewed changes
wfaderhold21
wfaderhold21 commented Feb 20, 2025
View reviewed changes
wfaderhold21
wfaderhold21 commented Feb 20, 2025
View reviewed changes
aamirshafi
aamirshafi reviewed Feb 20, 2025
View reviewed changes
Sergei-Lebedev
Sergei-Lebedev reviewed Feb 26, 2025
View reviewed changes
src/core/ucc_context.c Outdated
if (flags == UCC_MEM_MAP_IMPORT || flags == UCC_MEM_MAP_IMPORT_OFFLOAD) {
return ucc_mem_map_import(context, flags, params, memh_size, memh);
} else {
if (params->n_segments > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need to support multiple mapping at a time in future? what is a use case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A use case would include onesided alltoall where both the destination and global work buffer are associated with the destination memory handles. This would allow use of the current algorithm implementation in UCC already. However, implementing this may not be clean...

@wfaderhold21 wfaderhold21 force-pushed the topic/explicit-mem-map-impl branch from 23d5050 to 9691c1f Compare March 4, 2025 21:52
@wfaderhold21
Copy link
Collaborator Author

@Sergei-Lebedev Thank you for the feedback. I believe I have addressed all your concerns. Please let me know if I haven't.

@wfaderhold21
Copy link
Collaborator Author

@janjust do we need more code review concerning this PR or can we apply the completed label?

@Sergei-Lebedev
Copy link
Contributor

@Sergei-Lebedev Thank you for the feedback. I believe I have addressed all your concerns. Please let me know if I haven't.

thanks, I reviewed only part of this PR since it's big. I will continue reviewing this week.

samnordmann
samnordmann reviewed Mar 5, 2025
View reviewed changes
src/components/cl/basic/cl_basic.c Outdated
@@ -10,6 +10,13 @@ ucc_status_t ucc_cl_basic_get_lib_attr(const ucc_base_lib_t *lib,
ucc_base_lib_attr_t *base_attr);
ucc_status_t ucc_cl_basic_get_context_attr(const ucc_base_context_t *context,
ucc_base_ctx_attr_t *base_attr);
ucc_status_t ucc_cl_basic_mem_map(const ucc_base_context_t *context, int type,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave it up to you but these declarations could even go into the macro UCC_BASE_IFACE_DECLARE, it would save a lot of duplication

ferrol aderholdt added 20 commits May 9, 2025 06:54
REVIEW: address feedback
7af6430
REVIEW: address feedback
41d0240
API: update mem_map with exchange size
0d39b39
CORE: Add support for ucc_mem_map/unmap
e79683b 
CL/BASIC: add support for ucc_mem_map/unmap

CL/HIER: add support for ucc_mem_map/unmap

CL/DOCA_UROM: add support for ucc_mem_map/unmap

TL/CUDA: add support for ucc_mem_map/unmap

TL/MLX5: add support for ucc_mem_map/unmap

TL/NCCL: add support for ucc_mem_map/unmap

TL/RCCL: add support for ucc_mem_map/unmap

TL/SELF: add support for ucc_mem_map/unmap

TL/SHARP: add support for ucc_mem_map/unmap
TL/UCP: add support for ucc_mem_map/unmap
426f270 
TL/UCP: add support for communication with memmap

REVIEW: various fixes

TL/UCP: Enable multiple packed buffers
CODESTYLE: fix code style
c0c096d
REVIEW: address failures
625edab
REVIEW: various fixes for DPU
6033818
REVIEW: ci fixes
6e3b6b2
REVIEW: extra fixes
9a6e1ab
REVIEW: fix unmap
e0ca92c
REVIEW: fix a2a without memmap
30d1c33
CODESTYLE: nolint
fd25887
TL/SHARP: rollback NOLINT
4d17fc8
REVIEW: address feedback
ec1c622
REVIEW: address clang-tidy
080eac6
REVIEW: fix context creation
8aef6e4
REVIEW: address feedback
8128332
REVIEW: fix compile
a1ffa62
REVIEW: fix clang-tidy
c8ec986
@wfaderhold21 wfaderhold21 force-pushed the topic/explicit-mem-map-impl branch from 2f469f5 to bb5ee28 Compare May 9, 2025 13:55
REVIEW: address feedback
f88c054 
TL/UCP: additional support for memh in onesided alltoall
@wfaderhold21 wfaderhold21 force-pushed the topic/explicit-mem-map-impl branch from bb5ee28 to f88c054 Compare May 9, 2025 14:13
@wfaderhold21
Copy link
Collaborator Author

@Sergei-Lebedev Thanks again for the time and comments related to this PR. I've updated the PR based on what you've suggested and answered your comments. Please let me know if there is anymore feedback.

@wfaderhold21 wfaderhold21 mentioned this pull request May 21, 2025
Closed
Sergei-Lebedev
Sergei-Lebedev approved these changes May 22, 2025
View reviewed changes
src/components/tl/ucp/tl_ucp_context.c
tl_error(ctx->super.super.lib, "unable to pack memory handle without TL handles");
return UCC_ERR_INVALID_PARAM;
}
if (ctx->cfg.exported_memory_handle && mode == UCC_MEM_MAP_MODE_EXPORT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be UCC_MEM_MAP_MODE_EXPORT_OFFLOAD?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UCC_MEM_MAP_MODE_EXPORT_OFFLOAD is used to perform an export of a memory handle on an offload device (e.g., DPU). In that case, there's no need to pack the UCP memory handle on the DPU. It can just pack rkeys.

@Sergei-Lebedev
Copy link
Contributor

@wfaderhold21 thank you for all the hard work you've put into this pull request — it's greatly appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sergei-Lebedev Sergei-Lebedev Sergei-Lebedev approved these changes

@aamirshafi aamirshafi aamirshafi approved these changes

@samnordmann samnordmann samnordmann approved these changes

@janjust janjust Awaiting requested review from janjust

@nsarka nsarka Awaiting requested review from nsarka

Assignees
No one assigned
Labels
Arch-Review-Completed Code-Review-Completed Ready-for-Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wfaderhold21 @swx-jenkins3 @manjugv @Sergei-Lebedev @aamirshafi @samnordmann @janjust