Skip to content

mtl/ofi: Remove dependence on libfabric's definition of container_of macro #12726

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

Closed
wants to merge 2 commits into from

Conversation

lrbison
Copy link
Contributor

@lrbison lrbison commented Aug 1, 2024

Two commits:

  • Remove dependence on libfabric's definition of container_of macro. They plan to remove it from libfabric's main-branch with 2.0 deprecations ofiwg/libfabric#10250.
  • Remove a useless circular include I saw while in that code.

lrbison added 2 commits August 1, 2024 14:04
Previously mtl_ofi_request.h inadvertently used container_of as
defined by libfabric in include/rdma/fabric.h, however libfabric is
removing that symbol from their headers as part of changes supporting
libfabric 2.0 API. (see ofiwg/libfabric#10250).

This change defines our own (compatible) container_of macro to remove
the external dependence, but only if it is not already defined.

Signed-off-by: Luke Robison <[email protected]>
mtl_ofi_request.h previously included mtl_ofi.h, however mtl_ofi.h is
in fact the only file to include mtl_ofi_request.h.  The standard
header guards (MTL_OFI_H_HAS_BEEN_INCLUDED) prevented true circular
dependency, and makes the include statement useless.  This commit
removes the usless include.

Signed-off-by: Luke Robison <[email protected]>
@wenduwan
Copy link
Contributor

wenduwan commented Aug 1, 2024

I have a contending PR #12724

@lrbison which approach do you prefer?

@lrbison
Copy link
Contributor Author

lrbison commented Aug 1, 2024

@wenduwan oh, sorry -- I missed yours.

I don't have a strong preference, they both accomplish the same thing. I slightly prefer mine because it has less potential for wider impact and doesn't add an extra header file, but it does repeat itself repeat itself slightly.

@wenduwan
Copy link
Contributor

wenduwan commented Aug 1, 2024

I didn't remove the circular dependency in my PR though.

@lrbison
Copy link
Contributor Author

lrbison commented Aug 1, 2024

Meh, the circular dependency is just a one-line cleanup.

Side note: seems like there is some network problem in the CI runners. Both ROCM and mpi4py jobs timed out getting resources from different domains. The rest of them are happy.

@lrbison
Copy link
Contributor Author

lrbison commented Aug 1, 2024

Canceled in favor of #12724

@lrbison lrbison closed this Aug 1, 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.

2 participants