Skip to content
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.

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

freelist: separate out metadata from user data #600

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

rauteric
Copy link
Contributor

@rauteric rauteric commented Sep 13, 2024

Updates the interface of freelist to return an elem struct that contains
a buffer pointer and (optionally) memory registration info. Also updates
sendrecv, rdma, and scheduler codes to use the new interface.

This separation paves the way for storing freelist user data in GPU
memory.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rauteric rauteric force-pushed the freelist-refactor branch 2 times, most recently from 3664cb9 to 467a451 Compare September 13, 2024 04:24
Copy link
Contributor

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

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

I think having two different interfaces is a mistake. We should have one way to allocate / free freelist entries, whether or not they're registered.

@rauteric
Copy link
Contributor Author

I think having two different interfaces is a mistake. We should have one way to allocate / free freelist entries, whether or not they're registered.

Ok. My initial plan was to have entry_alloc() return both the buffer and an optional memory registration, but I couldn't make that work because we need to keep track of the entry structures, which are allocated separately.

So basically, we're going to end up with the "mr" interface variant (that returns an elem_t) in this PR being used for both the simple and complex freelists, That is,

static inline struct nccl_ofi_freelist_elem_t *nccl_ofi_freelist_entry_alloc
					       (nccl_ofi_freelist_t *freelist)

and

static inline void nccl_ofi_freelist_entry_free(nccl_ofi_freelist_t *freelist,
						   struct nccl_ofi_freelist_elem_t *entry)

We'll need to update both SENDRECV and RDMA protocols to use this interface.

@rauteric rauteric changed the title freelist: separate out metadata from user data for MR freelists freelist: separate out metadata from user data Oct 4, 2024
@rauteric rauteric marked this pull request as ready for review October 4, 2024 00:11
@rauteric rauteric requested a review from a team as a code owner October 4, 2024 00:11
@rauteric rauteric force-pushed the freelist-refactor branch 2 times, most recently from f2cba68 to 9e03725 Compare October 10, 2024 18:25
@rauteric
Copy link
Contributor Author

Rebased on master

include/nccl_ofi_freelist.h Outdated Show resolved Hide resolved
include/nccl_ofi_freelist.h Show resolved Hide resolved
src/nccl_ofi_rdma.c Outdated Show resolved Hide resolved
src/nccl_ofi_rdma.c Outdated Show resolved Hide resolved
@rauteric
Copy link
Contributor Author

Addressed feedback from Brian

@rauteric
Copy link
Contributor Author

Rebase on master

@rauteric
Copy link
Contributor Author

rauteric commented Dec 4, 2024

Rebase on master

@rauteric
Copy link
Contributor Author

rauteric commented Dec 4, 2024

bot:aws:retest

From CI:

Cloning into 'aws-ofi-nccl'...
fatal: unable to access 'https://github.com/aws/aws-ofi-nccl.git/': The requested URL returned error: 502

Not sure what happened there; trying again.

Updates the interface of freelist to return an elem struct that contains
a buffer pointer and (optionally) memory registration info. Also updates
sendrecv, rdma, and scheduler codes to use the new interface.

This separation paves the way for storing freelist user data in GPU
memory.

Signed-off-by: Eric Raut <[email protected]>
Copy link
Contributor

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

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

In the end, I really hate this interface and think I was wrong about having the same interface for both use cases. But I'd rather do that in C++ where we just use a builtin PMR interface for freelists, rather than continuing to iterate here. So let's ship it and move on.

@xwangxin xwangxin requested a review from bwbarrett December 18, 2024 21:58
Copy link
Contributor

@aws-nslick aws-nslick left a comment

Choose a reason for hiding this comment

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

did not test myself, but looks good to me.

@rauteric rauteric merged commit 2e26a5f into aws:master Dec 18, 2024
23 checks passed
@rauteric rauteric deleted the freelist-refactor branch December 18, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants