Skip to content

prov/verbs: Automatic GPU-NIC affinity#12203

Open
gad-arbel wants to merge 5 commits intoofiwg:mainfrom
gad-arbel:automatic_gpu_nic_affinity
Open

prov/verbs: Automatic GPU-NIC affinity#12203
gad-arbel wants to merge 5 commits intoofiwg:mainfrom
gad-arbel:automatic_gpu_nic_affinity

Conversation

@gad-arbel
Copy link
Copy Markdown

No description provided.

@gad-arbel gad-arbel closed this Apr 30, 2026
@gad-arbel gad-arbel deleted the automatic_gpu_nic_affinity branch April 30, 2026 16:01
@gad-arbel gad-arbel restored the automatic_gpu_nic_affinity branch April 30, 2026 16:02
@gad-arbel gad-arbel reopened this Apr 30, 2026
Copy link
Copy Markdown
Contributor

@j-xiong j-xiong left a comment

Choose a reason for hiding this comment

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

In general, looks good, see the nitpicks in line.

Please sign the commits (use git commit -s) otherwise the DCO check won't pass.

It would be nice to have a few sentences in the commit message to describe what the commit does. For the first commit, consider changing the title to prov/verbs: Add infrastructure for ..., and the commit message can be a little bit more detailed than the rest.

The appveyor CI failure is due to setenv and unsetenv not being available on Windows. Need to implement wrappers (using _putenv_s) in include/windows/osd.h.

I would prefer the new test being standalone instead of being part of the existing getinfo test. This way the new test can be configured to only run on machines that makes the test meaningful.

Comment thread prov/verbs/src/verbs_init.c Outdated
Comment on lines +70 to +71
.nic_affinity_policy = "none",
.affinity_device = NULL,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Try to keep the = aligned like the code segment above.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think we have different tab width. Hope that I fixed what you meant.

Comment thread prov/verbs/src/verbs_info.c Outdated
Comment on lines +2115 to +2125
if (!ancestor) {
return VRB_PROXIMITY_UNKNOWN;
}

if (ancestor->type == HWLOC_OBJ_BRIDGE) {
return VRB_PROXIMITY_BRIDGE;
}

if (ancestor->type == HWLOC_OBJ_PACKAGE) {
return VRB_PROXIMITY_PACKAGE;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Convention here is not to use { and } around single line body for conditional statements.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread prov/verbs/src/verbs_info.c Outdated
Comment on lines +2138 to +2143
if (nic_a->proximity < nic_b->proximity) {
return -1;
}
if (nic_a->proximity > nic_b->proximity) {
return 1;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread prov/verbs/src/verbs_info.c Outdated
Comment on lines +2226 to +2231
for (cur = *info; cur; cur = cur->next) {
entries_count++;
}
if (entries_count == 0) {
return FI_SUCCESS;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

@gad-arbel gad-arbel force-pushed the automatic_gpu_nic_affinity branch from d48ba04 to c66e17f Compare May 4, 2026 08:11
Introduce a NIC affinity framework to enable NIC ordering in fi_getinfo()
results based on GPU-NIC proximity. The 'none' policy preserves current
behavior (no reordering) and serves as the default for backward
compatibility. It establishes the infrastructure for policy-based handlers
that will be added in subsequent commits.

Signed-off-by: Gad Arbel <gad.arbel@intel.com>
@gad-arbel gad-arbel force-pushed the automatic_gpu_nic_affinity branch 3 times, most recently from c3fb7b3 to f936ae3 Compare May 4, 2026 15:01
@j-xiong
Copy link
Copy Markdown
Contributor

j-xiong commented May 4, 2026

The setenv/unsetenv wrappers for windows need to be duplicated in fabtests as well (fabtests/include/windows/osd.h).

Can we make a new test for the affinity settings and leave the original getinfo test unchanged?

@gad-arbel gad-arbel force-pushed the automatic_gpu_nic_affinity branch from f936ae3 to 887cb0d Compare May 5, 2026 13:36
@gad-arbel
Copy link
Copy Markdown
Author

gad-arbel commented May 5, 2026

The setenv/unsetenv wrappers for windows need to be duplicated in fabtests as well (fabtests/include/windows/osd.h).

Can we make a new test for the affinity settings and leave the original getinfo test unchanged?

Thanks for the Windows tip - that was helpful.
Regarding the test separation, I've created the new fi_nic_affinity_test.c file and it compiles and runs successfully. I'm currently working through some cleanup issues in the new test file, but the functionality appears to be working correctly. I'll have it polished and ready for review shortly.

@gad-arbel gad-arbel force-pushed the automatic_gpu_nic_affinity branch 2 times, most recently from 581dcc6 to 84fbcdb Compare May 6, 2026 10:03
gad-arbel added 4 commits May 7, 2026 02:01
Implement the 'manual' policy that allows users to specify explicit
GPU-to-NIC mappings via a configuration file. This policy reads a
text-based config file containing PCI address to NIC name mappings
and reorders fi_getinfo() results to prioritize the mapped NIC.

Signed-off-by: Gad Arbel <gad.arbel@intel.com>
Implement the 'auto' policy that automatically detects and prioritizes
NICs based on PCIe topology proximity to the target GPU device using
hwloc. This policy calculates PCIe proximity levels between the GPU and
each NIC, then reorders the fi_getinfo() list to place closer NICs first.

Signed-off-by: Gad Arbel <gad.arbel@intel.com>
Add hwloc library detection and linking to the verbs provider build
system. Hwloc is optional and used by the 'auto' policy to query PCIe
topology for automatic GPU-NIC proximity detection. When hwloc is not
available, the 'auto' policy falls back to 'none' behavior.

Signed-off-by: Gad Arbel <gad.arbel@intel.com>
Add test suite for GPU-NIC affinity policies. Tests validate that the
affinity feature does not break existing functionality by checking failure
states and verifying that all original fi_info entries are preserved in the
result list. Tests do not verify reordering correctness - the goal is to
ensure that in the worst case, users receive a different permutation of
results rather than missing or corrupted data.

Signed-off-by: Gad Arbel <gad.arbel@intel.com>
@gad-arbel gad-arbel force-pushed the automatic_gpu_nic_affinity branch from 84fbcdb to 9e2efa6 Compare May 7, 2026 09:11
@j-xiong
Copy link
Copy Markdown
Contributor

j-xiong commented May 9, 2026

- name:   fi_nic_affinity_test -p "tcp"
  timestamp: 20260507-201418+0000
  result: Fail
  time:   1
  server_cmd:  fi_nic_affinity_test -p "tcp"
  server_stdout: |
    fi_nic_affinity_test: invalid option -- 'p'
    Usage: fi_nic_affinity_test [options]
    Unit tests for GPU-NIC affinity
    Options:
      -h	Display this help message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants