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/MCAST: adding ip over ib mcast helper functions as well as some of the environment variables #861

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

MamziB
Copy link
Collaborator

@MamziB MamziB commented Oct 25, 2023

TL/MLX5: adding ip over ib mcast helper functions as well as some of the environment variables

@MamziB MamziB requested a review from samnordmann October 25, 2023 19:58
@MamziB MamziB self-assigned this Oct 25, 2023
@MamziB MamziB requested a review from Sergei-Lebedev October 25, 2023 19:58
@MamziB MamziB force-pushed the mamzi/mcast-merge-6-1 branch from e23dd95 to 0e09a78 Compare October 25, 2023 20:01
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.

It looks good to me. I just left some minor comments

src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.h Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.c Outdated Show resolved Hide resolved
@MamziB MamziB force-pushed the mamzi/mcast-merge-6-1 branch from 0e09a78 to ffd47f9 Compare October 31, 2023 21:19
@MamziB
Copy link
Collaborator Author

MamziB commented Oct 31, 2023

@samnordmann thanks for the comments. I have updated the commit.

src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/mcast/tl_mlx5_mcast_helper.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/tl_mlx5.c Outdated Show resolved Hide resolved
src/components/tl/mlx5/tl_mlx5.c Show resolved Hide resolved
@MamziB MamziB force-pushed the mamzi/mcast-merge-6-1 branch from ffd47f9 to d8d3227 Compare November 1, 2023 20:55
@MamziB
Copy link
Collaborator Author

MamziB commented Nov 1, 2023

@samnordmann @Sergei-Lebedev Thank you guys for the helpful comments. I updated the commit and responded to the comments. Thank you.

@MamziB MamziB force-pushed the mamzi/mcast-merge-6-1 branch from d8d3227 to 1d02f84 Compare November 3, 2023 17:46
@MamziB
Copy link
Collaborator Author

MamziB commented Nov 3, 2023

@Sergei-Lebedev @samnordmann thanks for the review. The commit is ready.
Thanks, Mamzi

@MamziB
Copy link
Collaborator Author

MamziB commented Nov 6, 2023

@samnordmann I can print a warning if fclose() fail. It will not affect the mcast init if it fclose() fails. Furthermore, other parts of UCC also do not check the return value. I will update the commit.

@MamziB MamziB force-pushed the mamzi/mcast-merge-6-1 branch from 1d02f84 to 7fb7f36 Compare November 6, 2023 15:30
@MamziB
Copy link
Collaborator Author

MamziB commented Nov 6, 2023

@samnordmann I updated the commit.
Thank you. Mamzi

@samnordmann
Copy link
Collaborator

@samnordmann I can print a warning if fclose() fail. It will not affect the mcast init if it fclose() fails. Furthermore, other parts of UCC also do not check the return value. I will update the commit.

There could be an error also at file opening, and in this case it will affect the mcast init

@MamziB
Copy link
Collaborator Author

MamziB commented Nov 6, 2023

@samnordmann fopen() checks for error, if return is NULL, we are handling it

@MamziB MamziB force-pushed the mamzi/mcast-merge-6-1 branch from 7fb7f36 to c43714c Compare November 6, 2023 17:05
@MamziB
Copy link
Collaborator Author

MamziB commented Nov 6, 2023

@samnordmann I updated the commit, and I used the same return code as when we cannot open the file for fclose().

@MamziB MamziB force-pushed the mamzi/mcast-merge-6-1 branch from c43714c to 7798823 Compare November 13, 2023 19:34
@Sergei-Lebedev
Copy link
Contributor

@MamziB I will merge this PR once CI is back to normal, i.e. need to merge this #877 first

@Sergei-Lebedev Sergei-Lebedev merged commit ac37cfa into openucx:master Nov 14, 2023
9 of 11 checks passed
B-a-S pushed a commit to B-a-S/ucc that referenced this pull request Jan 4, 2024
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