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

TL/MLX5: rcache and p2p lib for mcast #826

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

MamziB
Copy link
Collaborator

@MamziB MamziB commented Aug 22, 2023

TL/MLX5: adding rcache and a p2p library for mcast

@janjust

src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@samnordmann samnordmann left a comment

Choose a reason for hiding this comment

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

Hey @MamziB ! Thanks for the PR. I left some general comments.

Is it normal that the new files have not been added to the Makefile?
It would be a good thing to compile those files in order to spot compilation errors and also to make the Linter run on the files

src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
@MamziB
Copy link
Collaborator Author

MamziB commented Aug 23, 2023

Hi @samnordmann Thank you so much for the helpful comments. I am planning to add these new files to Makerile.am as soon as all the dependencies are in place. Right now since this PR is dependent to many codes that are not included in this PR, we cannot compile it. In the next few PRs we will be able to compile these and run Linter over it.

@samnordmann
Copy link
Collaborator

Hi @samnordmann Thank you so much for the helpful comments. I am planning to add these new files to Makerile.am as soon as all the dependencies are in place. Right now since this PR is dependent to many codes that are not included in this PR, we cannot compile it. In the next few PRs we will be able to compile these and run Linter over it.

Hey Mamzi, ok, but can you briefly clarify though what this pr depends on?
It seemed to me that it does not depend so much on external code, except maybe some type declaration (e.g. ucc_tl_mlx5_mcast_coll_context_t ucc_tl_mlx5_mcast_p2p_completion_obj_t). Anyway, it would be useful for the review to have those declarations, and also it would allow to compile.

Am I missing other difficulties?

@MamziB
Copy link
Collaborator Author

MamziB commented Aug 23, 2023

@samnordmann Thanks for the note. Sure, let me resolve the dependencies and add a new commit.

@MamziB
Copy link
Collaborator Author

MamziB commented Aug 24, 2023

@Sergei-Lebedev @samnordmann Thank you all for your constructive comments. I appreciate it. Please kindly take a look at the new commit that has addressed your feedback.

src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
@MamziB
Copy link
Collaborator Author

MamziB commented Aug 28, 2023

@Sergei-Lebedev Thanks for the second set of comments. Please take a look at the new commit. Thank you

@MamziB MamziB force-pushed the mamzi/mcast-merge-4 branch from 5647f56 to 26c4912 Compare August 28, 2023 19:25
@samnordmann samnordmann self-requested a review September 3, 2023 11:22
Copy link
Collaborator

@samnordmann samnordmann left a comment

Choose a reason for hiding this comment

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

Thanks @MamziB for the revisions. It looks good to me. I'm only leaving a couple of minor comments.
Can you please fix the Linter in github's checks ?

src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_rcache.c Outdated Show resolved Hide resolved
@MamziB MamziB force-pushed the mamzi/mcast-merge-4 branch from 26c4912 to 67ea8e4 Compare September 5, 2023 19:12
@MamziB
Copy link
Collaborator Author

MamziB commented Sep 5, 2023

@samnordmann @Sergei-Lebedev Hi Guys I have resolved/answered your feedback. I also converted all the commits into a single commit that can be merged into the master branch. Thanks again for your feedback.

@samnordmann samnordmann self-requested a review September 6, 2023 11:25
@MamziB MamziB force-pushed the mamzi/mcast-merge-4 branch from 67ea8e4 to 68303fb Compare September 6, 2023 18:19
@MamziB MamziB self-assigned this Sep 6, 2023
@MamziB MamziB force-pushed the mamzi/mcast-merge-4 branch from 68303fb to b78ec4c Compare September 6, 2023 19:17
@MamziB
Copy link
Collaborator Author

MamziB commented Sep 6, 2023

Thanks guys for the comments. @Sergei-Lebedev I have resolved your new comments and then I updated the commit itself.

src/components/tl/mlx5/mcast/tl_mlx5_mcast.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/p2p/ucc_tl_mlx5_mcast_p2p.c Outdated Show resolved Hide resolved
@janjust
Copy link
Collaborator

janjust commented Sep 8, 2023

@Sergei-Lebedev are all passing checks a requirement for mergers? I see some tests which failed in PRs already merged?
Thank you @Sergei-Lebedev and @samnordmann for detailed reviews. And a quick turnaround on the feedback.

@MamziB MamziB force-pushed the mamzi/mcast-merge-4 branch from b78ec4c to 0d1c8ed Compare September 8, 2023 18:54
@MamziB
Copy link
Collaborator Author

MamziB commented Sep 8, 2023

@Sergei-Lebedev Thanks sergei for the new comments. I appreciate it. I have updated the commit with the new changes.

@Sergei-Lebedev
Copy link
Contributor

@Sergei-Lebedev are all passing checks a requirement for mergers? I see some tests which failed in PRs already merged? Thank you @Sergei-Lebedev and @samnordmann for detailed reviews. And a quick turnaround on the feedback.

Yes, all checks are required unless there is known issue

@Sergei-Lebedev Sergei-Lebedev enabled auto-merge (squash) September 11, 2023 10:45
@Sergei-Lebedev Sergei-Lebedev merged commit 745474a into openucx:master Sep 11, 2023
7 of 9 checks passed
nsarka pushed a commit to nsarka/ucc that referenced this pull request Oct 24, 2023
nsarka pushed a commit to nsarka/ucc that referenced this pull request Oct 24, 2023
janjust pushed a commit to janjust/ucc that referenced this pull request Jan 31, 2024
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