Skip to content

[jaccl] Detect dead RDMA links instead of crashing - #4767

Open
ipazarbasi wants to merge 1 commit into
ml-explore:mainfrom
ipazarbasi:fix-4192
Open

ipazarbasi wants to merge 1 commit into
ml-explore:mainfrom
ipazarbasi:fix-4192

Conversation

@ipazarbasi

Copy link
Copy Markdown

After a cable pull, JACCL currently ignores completion errors, posts another receive on the dead QP, and crashes in tbt_post_recv. No completion may ever arrive on the peer, and it hangs.

qp->state only caches the last modify or query, so it never shows the QP going to ERR. To see a dead QP when no completion arrives, JACCL now calls ibv_query_qp. It is a syscall, so it only runs after a stall.

  • poll() checks the status of every completion and reports the most severe error.
  • poll() calls ibv_query_qp every 100 ms while stalled, and once more when a completion ends a stall longer than 2 ms, because a completion can report success on a dead QP. A QP in ERR or SQE is treated as dead.
  • All connections in a group share one failure latch which a failed post also trips
  • dispatch_wires waits for all wires, then rethrows the first exception. Before this patch, exceptions on pool threads were dropped.
  • Add device-free tests with a fake verbs provider

Behavior changes:

  • A failed ibv_post_send or ibv_post_recv now throws std::runtime_error instead of std::invalid_argument.
  • Groups stay failed after their first error, and every later call on them throws that error. The application must tear the failed group down.

Tested with two Macs over Thunderbolt 5 by pulling the cable at either end. Current code crashed in tbt_post_recv on every pull. The patched build raised a catchable error on both ranks in under 300 ms on 9 of 9 pulls (mesh and ring, all_sum and all_gather). all_sum timings from 1 KB to 64 MB match the current code within run-to-run noise.

Refs #4192

  • ☑️ I understand it is strictly prohibited to use AI to write PR description
  • AI usage disclosure: AI used for code review.

After a cable pull, JACCL currently ignores completion errors, posts
another receive on the dead QP, and crashes in `tbt_post_recv`. No
completion may ever arrive on the peer, and it hangs.

`qp->state` only caches the last modify or query, so it never shows the
QP going to ERR. To see a dead QP when no completion arrives, JACCL now
calls `ibv_query_qp`. It is a syscall, so it only runs after a stall.

* `poll()` checks the status of every completion and reports the most
  severe error.
* `poll()` calls `ibv_query_qp` every 100 ms while stalled, and once
  more when a completion ends a stall longer than 2 ms, because a
  completion can report success on a dead QP. A QP in `ERR` or `SQE` is
  treated as dead.
* All connections in a group share one failure latch which a failed post
  also trips
* `dispatch_wires` waits for all wires, then rethrows the first
  exception. Before this patch, exceptions on pool threads were dropped.
* Add device-free tests with a fake verbs provider

Behavior changes:
* A failed `ibv_post_send` or `ibv_post_recv` now throws
  `std::runtime_error` instead of `std::invalid_argument`.
* Groups stay failed after their first error, and every later call on
  them throws that error. The application must tear the failed group
  down.

Tested with two Macs over Thunderbolt 5 by pulling the cable at either
end. Current code crashed in `tbt_post_recv` on every pull. The patched
build raised a catchable error on both ranks in under 300 ms on 9 of 9
pulls (mesh and ring, `all_sum` and `all_gather`). `all_sum` timings
from 1 KB to 64 MB match the current code within run-to-run noise.

Refs ml-explore#4192

This branch has not been deployed

No deployments
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.

1 participant