Skip to content

Fix exploding idle ctxs array #13061

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

Merged
merged 1 commit into from
May 19, 2025
Merged

Conversation

zhongchen530
Copy link

@zhongchen530 zhongchen530 commented Jan 28, 2025

Fixes #13060 by ensuring strict matching between the requested option and option of the reused context.

Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

24c764b: fix exploding idle ctxs array

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

6917230: fix exploding idle ctxs array Signed-off-by: John ...

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

1 similar comment
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

6917230: fix exploding idle ctxs array Signed-off-by: John ...

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@zhongchen530 zhongchen530 changed the title fix exploding idle ctxs array Fix exploding idle ctxs array Jan 28, 2025
Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

This looks good to me but maybe someone more familiar with the oshmem component should take a look (@janjust)

Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

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

I don't understand how this fixes the issue as you replace a bitwise and (so any bits could match) with a strict matching equality. The & would trigger in all the cases where == is true, and more.

I reread the discussion in #13060 and I now think this is a correct patch but not for the reason claimed in the PR log, but because it ensures a strict matching on the context options. Additionally it also covers the case of selecting contexts with no options, which is a plus. I suggest you change the log to make this clear.

@zhongchen530
Copy link
Author

I don't understand how this fixes the issue as you replace a bitwise and (so any bits could match) with a strict matching equality. The & would trigger in all the cases where == is true, and more.

I reread the discussion in #13060 and I now think this is a correct patch but not for the reason claimed in the PR log, but because it ensures a strict matching on the context options. Additionally it also covers the case of selecting contexts with no options, which is a plus. I suggest you change the log to make this clear.

I apologize for the delayed reply. By PR log, do you mean my commit message or the description of this PR?

@jsquyres
Copy link
Member

By PR log, do you mean my commit message or the description of this PR?

I'm going to guess that @bosilca meant both: the git commit message and the PR description body.

@zhongchen530
Copy link
Author

By PR log, do you mean my commit message or the description of this PR?

I'm going to guess that @bosilca meant both: the git commit message and the PR description body.

I have made changes to both. Please let me know if it still needs changes. Will this PR eventually get merged?

@jsquyres
Copy link
Member

I have made changes to both. Please let me know if it still needs changes.

@bosilca Can you comment?

Will this PR eventually get merged?

That's the goal!

@janjust janjust merged commit bdd6a74 into open-mpi:main May 19, 2025
15 checks passed
@janjust
Copy link
Contributor

janjust commented May 19, 2025

@zhongchen530 Please cherry-pick to v5.0.x

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.

Issue in OSHMEM idle contexts reuse
5 participants