Skip to content

nixlbench: fix object storage device ID collisions across threads - #1638

Merged
aranadive merged 3 commits into
ai-dynamo:mainfrom
scality:fix/nixlbench-obj-device-id
Jun 4, 2026
Merged

aranadive merged 3 commits into
ai-dynamo:mainfrom
scality:fix/nixlbench-obj-device-id

Conversation

@oliviergaraud

@oliviergaraud oliviergaraud commented May 13, 2026 •

Copy link
Copy Markdown
Contributor

What?

Compute globally unique device IDs for object storage descriptors in both allocateMemory and exchangeIOV.

Why?

The device ID passed to initBasicDescObj was the inner loop index (i or iov.devId), so every thread generated the same set of IDs (e.g. {0, 1, 2}).

This could route all threads through the same backend connection, defeating multi-threaded parallelism.
Additionally, devidx was never incremented in the OBJ branch of exchangeIOV, so all remote descriptors used device ID 0.

How?

In both allocateMemory (local side) and exchangeIOV (remote side), compute the device ID as list_idx * num_devices + inner_idx, producing unique IDs across all threads (e.g. thread 0 gets {0,1,2}, thread 1 gets {3,4,5}).
Also add the missing devidx++ in exchangeIOV so the remote descriptors stay in sync with local ones.

Summary by CodeRabbit

  • Chores
    • Improved internal benchmark device mapping to tie resources more consistently to both worker slices and devices.
    • Refined remote IO descriptor construction to use explicit per-slice/per-device indexing.
    • Reduced mapping-related inconsistencies in multi-device benchmark runs to improve reliability and predictability.

The device ID passed to initBasicDescObj was the inner loop index,
causing all threads to generate the same set of device IDs. This could
route all threads through the same backend connection, defeating
multi-threaded parallelism. Compute a globally unique ID as
list_idx * num_devices + inner_idx in both allocateMemory (local side)
and exchangeIOV (remote side) so descriptors stay in sync.

Also fix devidx not being incremented in the OBJ branch of
exchangeIOV, which caused all remote descriptors to use device ID 0.

Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
@copy-pr-bot

copy-pr-bot Bot commented May 13, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown

👋 Hi oliviergaraud! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

@coderabbitai

coderabbitai Bot commented May 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

Failed to post review comments

📝 Walkthrough

Walkthrough

This PR updates OBJ-storage paths to compute composite device IDs as list_idx * num_devices + device_idx in allocateMemory and exchangeIOV, and passes that obj_dev_id (while retaining iov.len and iov.metaInfo) to initBasicDescObj.

Changes

OBJ Device ID Mapping Refactor

Layer / File(s) Summary
Composite device ID calculation in allocation and IO exchange
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
allocateMemory and exchangeIOV compute obj_dev_id = list_idx * num_devices + device_idx and pass it to initBasicDescObj; exchangeIOV uses iov.len and iov.metaInfo for object size/name instead of relying on iov.devId.

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers:

  • aranadive
  • ovidiusm
  • ofer
  • brminich

"A rabbit hops through code tonight,
counting lists and devices just right,
list times devices—IDs align,
object names and sizes fine,
a neat little hop and all's in sight." 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing device ID collisions in object storage across threads.
Description check ✅ Passed The description includes all required template sections (What, Why, How) with clear, detailed explanations of the problem and solution.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp Outdated
Replace range-based iterator loops that manually tracked list_idx and
devidx counters with index-based loops for clarity.

Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
@aranadive

Copy link
Copy Markdown
Contributor

/ok to test 1a2d02f

@aranadive

Copy link
Copy Markdown
Contributor

/build

@aranadive
aranadive enabled auto-merge (squash) May 28, 2026 05:57
@oliviergaraud

Copy link
Copy Markdown
Contributor Author

@ofer

ofer commented May 28, 2026

Copy link
Copy Markdown
Contributor

/ok to test 1a2d02f

@aranadive

Copy link
Copy Markdown
Contributor

Thanks @aranadive I cannot access the pages to see what's wrong with the tests that fail, how can I move forward ? http://nbuprod.blsm.nvidia.com/nbu-swx-nixl-main/blue/organizations/jenkins/nixl-ci-dl-gpu/detail/nixl-ci-dl-gpu/621/pipeline/ http://nbuprod.blsm.nvidia.com/nbu-swx-nixl-main/blue/organizations/jenkins/nixl-ci-gpu/detail/nixl-ci-gpu/1860/pipeline/

Yeah those are not visible outside of our org. For now they are unrelated to the PR. We will triage and let you know if its a PR issue.

@ofer

ofer commented May 28, 2026

Copy link
Copy Markdown
Contributor

/build

@ofer

ofer commented May 31, 2026

Copy link
Copy Markdown
Contributor

@oliviergaraud please rebase to pick up some CI fixes

@oliviergaraud

Copy link
Copy Markdown
Contributor Author

@oliviergaraud please rebase to pick up some CI fixes

done.

@ofer

ofer commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

/ok to test dd79daf

@ofer

ofer commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

/build

1 similar comment
@ofer

ofer commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

/build

@aranadive
aranadive merged commit d4c6d8e into ai-dynamo:main Jun 4, 2026
23 of 24 checks passed
@oliviergaraud
oliviergaraud deleted the fix/nixlbench-obj-device-id branch June 4, 2026 12:22

This branch was previously deployed

1 inactive deployment
SWX_AWS — dd79daf2 Deployed Jun 2, 2026 by copy-pr-bot[bot] via Run AWS Tests #7510
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.

3 participants