Skip to content

SBP-2: LoginSession as a std::variant state machine - #227

Merged
mrmidi merged 1 commit into
mrmidi:mainfrom
mhellevang:refactor/sbp2-login-fsm
Oct 23, 2026
Merged

mrmidi merged 1 commit into
mrmidi:mainfrom
mhellevang:refactor/sbp2-login-fsm

Conversation

@mhellevang

@mhellevang mhellevang commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to the #216 review. LoginSession is now a std::variant state machine, so the login ID lives only in the states that hold a login and the LoginID() == 0 sentinel is gone. The H4D assigns login_ID 0 (all 20 LUN reset ORBs in the #213 capture), so before this it got a Login instead of a Reconnect after every reset.

This PR keeps the wire behaviour the same, apart from the changes listed below. The places where we differed from Apple/Linux are fixed in a small PR on top (#228).

Your five points:

  1. States carry their data. LoginFsm::St::* in LoginStateMachine.hpp: LoggedIn{login, route}, Reconnecting{login, route}, LoggingOut{login, route}, and Suspended split into SuspendedHeld{login} (reconnect) and SuspendedNoLogin (log in). HeldLogin is the login response: login_ID, command block agent, reconnect_hold. RefreshTargets now just calls Resume(), and the state decides.
  2. One transition function. Transition(state, event, now) → {next, effects} is pure; LoginSession feeds it events and carries out the effects (writes, timers, bind, callbacks) in a fixed order. Stale write completions (older generation or another op) are dropped there instead of in scattered checks. The table, with Apple/Linux citations per row, is in documentation/sbp2-login-fsm.md.
  3. Threading. The variant isn't only touched from the driver queue: the probe user client calls StartLogin/ReleaseSession from its own queue (no QUEUENAME, no hop). Hopping it with DispatchSync looked like the FW-60 teardown trap, so LoginSession serializes itself instead: a small locked queue of work, run one piece at a time by whichever thread finds it idle. The lock covers only the queue, so callbacks still take the registry's lock without an AB-BA against OnBusReset. Observers read Snapshot(): state, generation and login ID in one atomic word.
  4. LoginState stays the reported enum. The registry, the bridge and the user client are unchanged apart from reading the snapshot.
  5. Tests. LoginStateMachineTests: one test per transition (45), including a held login with ID 0 reconnecting. Integration: SessionRegistryTests.LoginIdZeroReconnectsAfterRediscovery, plus executor tests (a callback calling back in, a request from another thread queued behind running work). The cross-thread test fails if the queue is removed. The existing LoginSessionTests pass unchanged apart from Reconnect() → Resume().

Behaviour changes (also in the doc):

  • After a reset, a login with login_ID 0 reconnects.
  • A logout after a reset that interrupted the login completes without a write. It used to send a logout ORB with login_ID 0.
  • A logout from Suspended allocates the management address spaces again. The reset had released them, so the logout ORB pointer addressed nothing.
  • A timer that fires after it was cancelled or re-armed is dropped. A write that completes inline no longer has its retry timer overwritten.
  • The reported generation follows every reset, and the reported login ID is 0 outside the states that hold one.
  • LoginCompleteParams no longer carries the status block (nothing read it).

Not covered: the user-client path still drives CommandExecutor/FetchAgent from its own queue, as before.

Not tested on hardware yet. Host suite: 3877 passing.

@mrmidi

mrmidi commented Oct 23, 2026

Copy link
Copy Markdown
Owner

OnBusReset holds the registry's IOLock while calling HandleBusReset, and the new executor can make that same thread run a Logout() queued by the user client, whose callback then calls IOLockLock on the same lock again, which IOLib.h documents as a deadlock. Call HandleBusReset after releasing the lock.

Now:
OnBusReset:
  IOLockLock(lock_)                ← driver thread holds lock_
  session->HandleBusReset()        ← this thread becomes the session's runner
     ...runs the queued Logout()
        → logout callback → IOLockLock(lock_)   ← same thread, already holds it → deadlock
  IOLockUnlock(lock_)

With the fix:
OnBusReset:
  IOLockLock(lock_)
  copy the session pointers into a local list
  executor->OnBusReset() for each (stays under the lock, as now)
  IOLockUnlock(lock_)              ← lock released
  for each session: HandleBusReset()   ← runner thread holds no lock
     ...runs the queued Logout()
        → logout callback → IOLockLock(lock_)   ← takes it normally

Could you check it please?

@mhellevang

Copy link
Copy Markdown
Contributor Author

OnBusReset holds the registry's IOLock while calling HandleBusReset, and the new executor can make that same thread run a Logout() queued by the user client, whose callback then calls IOLockLock on the same lock again, which IOLib.h documents as a deadlock. Call HandleBusReset after releasing the lock.

Now:
OnBusReset:
  IOLockLock(lock_)                ← driver thread holds lock_
  session->HandleBusReset()        ← this thread becomes the session's runner
     ...runs the queued Logout()
        → logout callback → IOLockLock(lock_)   ← same thread, already holds it → deadlock
  IOLockUnlock(lock_)

With the fix:
OnBusReset:
  IOLockLock(lock_)
  copy the session pointers into a local list
  executor->OnBusReset() for each (stays under the lock, as now)
  IOLockUnlock(lock_)              ← lock released
  for each session: HandleBusReset()   ← runner thread holds no lock
     ...runs the queued Logout()
        → logout callback → IOLockLock(lock_)   ← takes it normally

Could you check it please?

Oops, you're right! Thank you

@mrmidi
mrmidi merged commit 9a23351 into mrmidi:main Oct 23, 2026
2 checks passed
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