Repository navigation
[Bugfix] Recover classic RDMA contexts when PORT_ACTIVE is missed - #4715
staryxchen merged 6 commits into
Conversation
A classic WorkerPool that disables its context on PORT_ERR only comes back through the PORT_ACTIVE event path. One missed event left the rail dead for the process lifetime. Poll ibv_query_port at 1 Hz like TENT and reuse the existing delayed GID probe. Fixes kvcache-ai#4694 Signed-off-by: YDMY_Kirito <202200820046@mail.sdu.edu.cn>
|
@huangxiaobai-ydmy no worries, thanks for working on it so quickly |
CI Check code format wants the asm declaration broken the same way as wrapRefreshCurrentGid. Signed-off-by: YDMY_Kirito <202200820046@mail.sdu.edu.cn>
|
@ironashram thanks for letting me keep this. |
| << " is now active after recovery delay"; | ||
| } | ||
|
|
||
| void WorkerPool::resumePausedContext() { |
There was a problem hiding this comment.
resumePausedContext() is documented as the safety net for a missed PORT_ACTIVE, but it polls every inactive context with recovery_activate_after_ns_ == 0 and breaker_reactivate_after_ns_ == 0. handleContextEvent() leaves that same state for DEVICE_FATAL, CQ_ERR, WQ_FATAL, PORT_ERR, and LID_CHANGE, and resetContextBreaker(false) clears the breaker deadline, so the breaker check does not exclude the other four.
A port that is still IBV_PORT_ACTIVE then arms the 30 s GID probe and publishes the RNIC again. That probe does not rebuild the CQ, so a CQ_ERR context comes back on the CQ that failed, and a DEVICE_FATAL context comes back whenever ibv_query_port still returns ACTIVE.
Please remember the disabling event and run this poll only for PORT_ERR. A test for each of the other four, with the port probe returning IBV_PORT_ACTIVE, should leave recovery_activate_after_ns_ == 0 and active() == false.
There was a problem hiding this comment.
@staryxchen thanks. The poll now records the disabling event and runs only after PORT_ERR. DEVICE_FATAL, CQ_ERR, WQ_FATAL, and LID_CHANGE stay inactive when the port probe returns ACTIVE; the new rail-state test covers each of them.
The 1 Hz poll treated every fatal async event like a missed PORT_ACTIVE. Remember the disabling event and arm the GID probe only after PORT_ERR. Signed-off-by: YDMY_Kirito <202200820046@mail.sdu.edu.cn>
The failed DFS client test is outside this PR. Push an empty commit so CI runs again on the current main merge. Signed-off-by: YDMY_Kirito <202200820046@mail.sdu.edu.cn>
|
@staryxchen the latest CI failure is CTest in mooncake-store: master_service_evict_scenario_test and dfs_sync_client_test. worker_pool_rail_state_test passed. This PR does not touch those store tests, and I cannot rerun the job. Could someone with permission rerun CTest? |
|
@staryxchen the change from your review is on 2575747. The poll records the disabling event and only resumes after PORT_ERR. DEVICE_FATAL, CQ_ERR, WQ_FATAL, and LID_CHANGE stay inactive when the port is still ACTIVE. CI on that commit is green. Please Approve when you have a moment. |
Description
Fixes #4694.
A classic
WorkerPoolthat processesIBV_EVENT_PORT_ERRdisables itscontext and disconnects endpoints. The only thing that arms recovery
afterwards is the
IBV_EVENT_PORT_ACTIVEbranch. If that event isnever read,
recovery_activate_after_ns_stays 0,maybeActivateRecoveredContext()is a no-op, and the rail stays inactive until the process restarts.
#3799 already added this safety net for TENT (
Workers::resumePausedContexts(),a 1 Hz
ibv_query_port). Classic still drains the async fd, but itnever got the poll.
What changed
RdmaContext::queryPortState()for the classic engine.fatal async event and no recovery is armed, query the port.
IBV_PORT_ACTIVE, callscheduleContextRecovery()so theexisting 30 s delayed GID probe from [TE] Improve RDMA NIC failover recovery #2959 still decides when the
RNIC is reused. Intermediate port states stay inactive.
that path.
WorkerPoolTestPeerand--wrapofqueryPortState. No IB device required.@ironashram this follows the fix you outlined on #4694. Sorry I opened it without checking with you first. You wrote that you were happy to send the PR yourself — if you still want to, say so and I will close this one so you can take it.
Test plan
worker_pool_rail_state_testcovers missedPORT_ACTIVE,breaker-owned pauses, and an already-armed event recovery.
worker_pool_rail_state_teston this PR.