Skip to content

Commit

Permalink
Fix session eviction handling of eviction hints. (#30806)
Browse files Browse the repository at this point in the history 
When we establish a session as a CASE responder, we try to allocate a new
session to listen to session establishments, and use the
just-established-session's peer ID as the eviction hint.

If we had a bunch of active sessions on a fabric, all to different nodes,
this would cause the just-established session to be evicted, since it matched
the hint.

The fix is to only consider sessions for eviction based on the hint if they are
either non-active or not unique sessions to the peer (at which point, the
just-established session should be last in priority order for eviction).

Fixes #30728
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Jan 19, 2024
1 parent 5133fb8 commit 1672364
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 9 deletions.
27 changes: 25 additions & 2 deletions src/transport/SecureSessionTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,31 @@ void SecureSessionTable::DefaultEvictionPolicy(EvictionPolicyContext & evictionC
return a.mNumMatchingOnPeer > b.mNumMatchingOnPeer;
}

int doesAMatchSessionHint = a.mSession->GetPeer() == evictionContext.GetSessionEvictionHint();
int doesBMatchSessionHint = b.mSession->GetPeer() == evictionContext.GetSessionEvictionHint();
// We have an evicton hint in two cases:
//
// 1) When we just established CASE as a responder, the hint is the node
// we just established CASE to.
// 2) When starting to establish CASE as an initiator, the hint is the
// node we are going to establish CASE to.
//
// In case 2, we should not end up here if there is an active session to
// the peer at all (because that session should have been used instead
// of establishing a new one).
//
// In case 1, we know we have a session matching the hint, but we don't
// want to pick that one for eviction, because we just established it.
// So we should not consider a session as matching a hint if it's active
// and is the only session to our peer.
//
// Checking for the "active" state in addition to the "only session to
// peer" state allows us to prioritize evicting defuct sessions that
// match the hint against other defunct sessions.
auto sessionMatchesEvictionHint = [&evictionContext](const SortableSession & session) -> int {
return session.mSession->GetPeer() == evictionContext.GetSessionEvictionHint() &&
(!session.mSession->IsActiveSession() || session.mNumMatchingOnPeer > 0);
};
int doesAMatchSessionHint = sessionMatchesEvictionHint(a);
int doesBMatchSessionHint = sessionMatchesEvictionHint(b);

//
// Sorting on Key4
Expand Down
2 changes: 1 addition & 1 deletion src/transport/SecureSessionTable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ class SecureSessionTable
* the session that is most ahead as the best candidate for eviction:
*
* - Key1: Sessions on fabrics that have more sessions in the table are placed ahead of sessions on fabrics
* with lesser sessions. We conclusively know that if a particular fabric has more sessions in the table
* with fewer sessions. We conclusively know that if a particular fabric has more sessions in the table
* than another, then that fabric is definitely over minimas (assuming a minimally sized session table
* conformant to spec minimas).
*
Expand Down
107 changes: 101 additions & 6 deletions src/transport/tests/TestSecureSessionTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,14 @@ void TestSecureSessionTable::CreateSessionTable(std::vector<SessionParameters> &
ScopedNodeId(1, sessionParams[i].mPeer.GetFabricIndex()), sessionParams[i].mPeer, CATValues(), static_cast<uint16_t>(i),
ReliableMessageProtocolConfig(System::Clock::Milliseconds32(0), System::Clock::Milliseconds32(0),
System::Clock::Milliseconds16(0)));
session.Value()->AsSecureSession()->mLastActivityTime = sessionParams[i].mLastActivityTime;
session.Value()->AsSecureSession()->mState = sessionParams[i].mState;

// Make sure we set up our holder _before_ the session goes into a state
// other than active, because holders refuse to hold non-active
// sessions.
mSessionList.push_back(Platform::MakeUnique<SessionNotificationListener>(session.Value()));

session.Value()->AsSecureSession()->mLastActivityTime = sessionParams[i].mLastActivityTime;
session.Value()->AsSecureSession()->mState = sessionParams[i].mState;
}
}

Expand Down Expand Up @@ -279,12 +283,44 @@ void TestSecureSessionTable::ValidateSessionSorting(nlTestSuite * inSuite, void
// This validates evicting from a table with equally loaded fabrics. In this scenario,
// bias is given to the fabric that matches that of the eviction hint.
//
// There are equal sessions to Node 2 as well as Node 3 in that fabric, so the Node
// that matches the session eviction hint will be selected, and in that, the older session.
// There is an equal number sessions to nodes 1, 2, and 3 in that fabric, so the Node
// that matches the session eviction hint will be selected.
//
// All the sessions in the table are defunct, because for unique active
// sessions eviction hints are ignored.
//
{
ChipLogProgress(SecureChannel,
"-------- Equal Fabrics Eviction (Equal # Sessions to Nodes, Hint Match On Fabric & Node) ---------");
ChipLogProgress(
SecureChannel,
"-------- Equal Fabrics Eviction (Single equal # Sessions to Nodes, Hint Match On Fabric & Node) ---------");

std::vector<SessionParameters> sessionParamList = {
{ { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kDefunct },
{ { 1, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kDefunct },
{ { 2, kFabric1 }, System::Clock::Timestamp(2), SecureSession::State::kDefunct },
{ { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kDefunct },
{ { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kDefunct },
{ { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kDefunct },
};

_this->CreateSessionTable(sessionParamList);
_this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 3);
}

//
// This validates evicting from a table with equally loaded fabrics. In this scenario,
// bias is given to the fabric that matches that of the eviction hint.
//
// There is an equal number sessions to nodes 1, 2, and 3 in that fabric, so the Node
// that matches the session eviction hint will be selected.
//
// All the peers in this table have two sessions to them, so that we pay
// attention to the eviction hint. The older of the two should be selected.
//
{
ChipLogProgress(
SecureChannel,
"-------- Equal Fabrics Eviction (Multiple equal # Sessions to Nodes, Hint Match On Fabric & Node) ---------");

std::vector<SessionParameters> sessionParamList = {
{ { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kActive },
Expand All @@ -293,12 +329,71 @@ void TestSecureSessionTable::ValidateSessionSorting(nlTestSuite * inSuite, void
{ { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kActive },
{ { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kActive },
{ { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive },
{ { 1, kFabric1 }, System::Clock::Timestamp(10), SecureSession::State::kActive },
{ { 1, kFabric2 }, System::Clock::Timestamp(4), SecureSession::State::kActive },
{ { 2, kFabric1 }, System::Clock::Timestamp(3), SecureSession::State::kActive },
{ { 3, kFabric1 }, System::Clock::Timestamp(8), SecureSession::State::kActive },
{ { 3, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive },
{ { 4, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kActive },
};

_this->CreateSessionTable(sessionParamList);
_this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 3);
}

//
// This validates evicting from a table with equally loaded fabrics. In this scenario,
// bias is given to the fabric that matches that of the eviction hint.
//
// There is an equal sessions to nodes 1, 2, and 3 in that fabric, and only
// one per node. Since all the sessions are active, the eviction hint's
// node id will be ignored and the oldest session on the fabric will be selected.
//
{
ChipLogProgress(SecureChannel,
"-------- Equal Fabrics Eviction (Equal # Sessions to Nodes, Hint Match On Fabric & Node, hint node "
"ignored) ---------");

std::vector<SessionParameters> sessionParamList = {
{ { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kActive },
{ { 1, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kActive },
{ { 2, kFabric1 }, System::Clock::Timestamp(2), SecureSession::State::kActive },
{ { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kActive },
{ { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kActive },
{ { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive },
};

_this->CreateSessionTable(sessionParamList);
_this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 2);
}

//
// This validates evicting from a table with equally loaded fabrics. In this scenario,
// bias is given to the fabric that matches that of the eviction hint.
//
// There is an equal sessions to nodes 1, 2, and 3 in that fabric, and only
// one per node. Since the hinted session is active, the eviction hint's
// node id will be ignored and the defunct session will be selected, even
// though it's the newest one.
//
{
ChipLogProgress(SecureChannel,
"-------- Equal Fabrics Eviction (Equal # Sessions to Nodes, Hint Match On Fabric & Node, hint node "
"ignored and state wins) ---------");

std::vector<SessionParameters> sessionParamList = {
{ { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kDefunct },
{ { 1, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kActive },
{ { 2, kFabric1 }, System::Clock::Timestamp(2), SecureSession::State::kActive },
{ { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kActive },
{ { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kActive },
{ { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive },
};

_this->CreateSessionTable(sessionParamList);
_this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 0);
}

//
// Similar to above, except that the eviction hint matches a given fabric (kFabric1) in the
// session table, but not any nodes. In this case, the oldest session in that fabric is selected
Expand Down

0 comments on commit 1672364

Please sign in to comment.