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.

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

core: 1.8 ABI compat #10385

Merged
merged 1 commit into from
Sep 17, 2024
Merged

core: 1.8 ABI compat #10385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/ofi_abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ extern "C" {
* name appended with the ABI version that it is compatible with.
*/

#define CURRENT_ABI "FABRIC_1.7"
#define CURRENT_ABI "FABRIC_1.8"
Copy link
Member

Choose a reason for hiding this comment

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

@j-xiong

This change indicates that the fi_fabric2() call is not aligned with the documentation in this file. Checking the code, it's using DEFAULT_SYMVER(... FABRIC_1.8) directly instead of CURRENT_SYMVER(). There were also changes to fi_getinfo / fi_freeinfo / fi_dupinfo as part of ABI 1.8 changes. The changes to those functions should be captured in the man page below as well.

If CURRENT_SYMVER() will no longer be used, that macro should be removed and the man page updated to reflect direct use of DEFAULT_SYMVER instead.

Copy link
Contributor

@shijin-aws shijin-aws Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@shefty @j-xiong If we are going to implement changes to fi_info today. Is it better to make it part of FABRIC_1.8 (since 2.0 is not released yet?), or we need to bump it as FABRIC_1.9?

Copy link
Contributor

Choose a reason for hiding this comment

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

We will keep it 1.8 for the 2.0 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shefty Using DEFAULT_SYMVER() here for the new function instead of CURRENT_SYMVER() has the advantage of showing the exact ABI version at the definition and not needing to change it again when the ABI version is bumped. I think we can remove CURRENT_SYMVER() definition and usage info from the header. If no objection, I will make that change in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to remove CURRENT_SYMVER


#if HAVE_ALIAS_ATTRIBUTE == 1
#define DEFAULT_SYMVER_PRE(a) a##_
Expand Down
8 changes: 8 additions & 0 deletions man/fabric.7.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,14 @@ attributes:
*fi_domain_attr*
: Added max_ep_auth_key

## ABI 1.8

ABI version starting with libfabric 2.0. Added new fi_fabric2 API call.
Added new fields to the following attributes:

*fi_domain_attr*
: Added max_group_id

j-xiong marked this conversation as resolved.
Show resolved Hide resolved
# SEE ALSO

[`fi_info`(1)](fi_info.1.html),
Expand Down
Loading