Skip to content

Fix OFI build warnings #13266

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
Merged
merged 2 commits into from
May 20, 2025
Merged

Fix OFI build warnings #13266

merged 2 commits into from
May 20, 2025

Conversation

@shijin-aws
Copy link
Contributor

@shijin-aws shijin-aws commented May 20, 2025 •
edited
Loading

This PR fixes the build warnings when compiling Open MPI with Libfabric 2.x, like

btl_ofi_component.c: In function ‘validate_info’:
btl_ofi_component.c:108:13: warning: 'FI_MR_BASIC' is deprecated
  108 |     if (!(mr_mode == FI_MR_BASIC || mr_mode == FI_MR_SCALABLE
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
btl_ofi_component.c:108:13: warning: 'FI_MR_SCALABLE' is deprecated
  108 |     if (!(mr_mode == FI_MR_BASIC || mr_mode == FI_MR_SCALABLE
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
btl_ofi_component.c: In function ‘mca_btl_ofi_init_device’:
btl_ofi_component.c:658:13: warning: 'FI_MR_BASIC' is deprecated
  658 |     if (ofi_info->domain_attr->mr_mode == FI_MR_BASIC

Removed the stale code snippet in mtl/ofi after talking with @hppritcha

Use internal macros for FI_MR_BASIC and FI_MR_SCALABLE to keep compabilities with old libfabric
and avoid breaking compilation with newer libfabrics

@github-actions
Copy link

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

30a4c2d: mtl/ofi: Remove special patch for gni provider

  • check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: 630a304f3bf059026e4b745d83dd52c2bf7d12c0

b20bf41: btl/ofi: Use internal macros for FI_MR_BASIC and F...

  • check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: fe1861a80291224b75f8c0c977684a471dcb6985

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

1 similar comment
@github-actions
Copy link

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

30a4c2d: mtl/ofi: Remove special patch for gni provider

  • check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: 630a304f3bf059026e4b745d83dd52c2bf7d12c0

b20bf41: btl/ofi: Use internal macros for FI_MR_BASIC and F...

  • check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: fe1861a80291224b75f8c0c977684a471dcb6985

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

shijin-aws added 2 commits May 20, 2025 04:18
@shijin-aws
btl/ofi: Use internal macros for FI_MR_BASIC and FI_MR_SCALABLE
281c6cd 
FI_MR_BASIC (1<<0) and FI_MR_SCALABLE (1<<1) are deprecated
since Libfabric 1.5 and the symbols will get removed in
future Libfabric 2.x versions. Use the internal mode bits
for backward compatibilities without breaking compilation
with newer libfabric.

Signed-off-by: Shi Jin <[email protected]>
@shijin-aws
mtl/ofi: Remove special patch for gni provider
1c67d90 
There should not be any Cray XC systems in production now
- which is where the GNI provider nominally functioned.

Meanwhile FI_MR_BASIC is deprecated since Libfabric 1.5
and will be droppped in future Libfabric 2.x versions.

Remove this special handler for gni provider due to
unnecessity and build warnings.

Signed-off-by: Shi Jin <[email protected]>
@shijin-aws shijin-aws force-pushed the ofi_build_warnings branch from 30a4c2d to 1c67d90 Compare May 20, 2025 04:18
lrbison
lrbison reviewed May 20, 2025
View reviewed changes
opal/mca/btl/ofi/btl_ofi_component.c
Comment on lines +50 to +54
* FI_MR_BASIC (1<<0) and FI_MR_SCALABLE (1<<1) are deprecated
* since Libfabric 1.5 and the symbols will get removed in
* future Libfabric 2.x versions. Use the internal mode bits
* for backward compatibilities without breaking compilation
* with newer libfabric.
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that a newer libfabric won't be reusing those old values? Otherwise FI_MR_SUPER_NEW_FANCY_MODE with value (1<<0) might be misinterpreted as FI_MR_BASIC in some future compile.

Copy link
Member

Choose a reason for hiding this comment

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

I would hope ofi wg would not make that mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there won't be newer libfabric to use these bits, it's forbidden in the development

Copy link
Member

Choose a reason for hiding this comment

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

okay I think in this case we will merge this PR. Thanks for cleaning our code a bit @shijin-aws

@hppritcha hppritcha self-requested a review May 20, 2025 17:02
hppritcha
hppritcha approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@hppritcha hppritcha left a comment

Choose a reason for hiding this comment

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

GNI provider is no longer supported and the vendor systems it worked on are EOL.

@hppritcha hppritcha merged commit 68b9015 into open-mpi:main May 20, 2025
15 checks passed
This was referenced May 20, 2025
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hppritcha hppritcha hppritcha approved these changes

@lrbison lrbison lrbison left review comments

Assignees

No one assigned

Labels

Target: main

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shijin-aws @hppritcha @lrbison