Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@erjiaqing
erjiaqing committed Jun 15, 2022
1 parent 2b22a06 commit fbeca4c
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 63 deletions.
80 changes: 38 additions & 42 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,12 @@ CHIP_ERROR InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeCon
if (commandHandler == nullptr)
{
ChipLogProgress(InteractionModel, "no resource for Invoke interaction");
aStatus = Protocols::InteractionModel::Status::Busy;
aStatus = Status::Busy;
return CHIP_ERROR_NO_MEMORY;
}
ReturnErrorOnFailure(
commandHandler->OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), aIsTimedInvoke));
aStatus = Protocols::InteractionModel::Status::Success;
aStatus = Status::Success;
return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -390,7 +390,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
reader.Init(aPayload.Retain());

ReadRequestMessage::Parser readRequestParser;
VerifyOrReturnError(readRequestParser.Init(reader) == CHIP_NO_ERROR, Protocols::InteractionModel::Status::Failure);
VerifyOrReturnError(readRequestParser.Init(reader) == CHIP_NO_ERROR, Status::Failure);

{
size_t requestedAttributePathCount = 0;
Expand All @@ -402,11 +402,11 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
TLV::TLVReader pathReader;
attributePathListParser.GetReader(&pathReader);
ReturnErrorCodeIf(TLV::Utilities::Count(pathReader, requestedAttributePathCount, false) != CHIP_NO_ERROR,
Protocols::InteractionModel::Status::Failure);
Status::InvalidAction);
}
else if (err != CHIP_ERROR_END_OF_TLV)
{
return Protocols::InteractionModel::Status::InvalidAction;
return Status::InvalidAction;
}
EventPathIBs::Parser eventpathListParser;
err = readRequestParser.GetEventRequests(&eventpathListParser);
Expand All @@ -415,17 +415,17 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
TLV::TLVReader pathReader;
eventpathListParser.GetReader(&pathReader);
ReturnErrorCodeIf(TLV::Utilities::Count(pathReader, requestedEventPathCount, false) != CHIP_NO_ERROR,
Protocols::InteractionModel::Status::Failure);
Status::InvalidAction);
}
else if (err != CHIP_ERROR_END_OF_TLV)
{
return Protocols::InteractionModel::Status::InvalidAction;
return Status::InvalidAction;
}

// The following cast is safe, since we can only hold a few tens of paths in one request.
Protocols::InteractionModel::Status checkResult = EnsureResourceForRead(
apExchangeContext->GetSessionHandle()->GetFabricIndex(), requestedAttributePathCount, requestedEventPathCount);
if (checkResult != Protocols::InteractionModel::Status::Success)
if (checkResult != Status::Success)
{
return checkResult;
}
Expand Down Expand Up @@ -480,13 +480,13 @@ CHIP_ERROR InteractionModelEngine::OnTimedRequest(Messaging::ExchangeContext * a
if (handler == nullptr)
{
ChipLogProgress(InteractionModel, "no resource for Timed interaction");
aStatus = Protocols::InteractionModel::Status::Busy;
aStatus = Status::Busy;
return CHIP_ERROR_NO_MEMORY;
}

// The timed handler takes over handling of this exchange and will do its
// own status reporting as needed.
aStatus = Protocols::InteractionModel::Status::Success;
aStatus = Status::Success;
apExchangeContext->SetDelegate(handler);
return handler->OnMessageReceived(apExchangeContext, aPayloadHeader, std::move(aPayload));
}
Expand Down Expand Up @@ -537,7 +537,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
{
using namespace Protocols::InteractionModel;

Protocols::InteractionModel::Status status = Protocols::InteractionModel::Status::Failure;
Protocols::InteractionModel::Status status = Status::Failure;

// Group Message can only be an InvokeCommandRequest or WriteRequest
if (apExchangeContext->IsGroupExchangeContext() &&
Expand Down Expand Up @@ -568,7 +568,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData))
{
ReturnErrorOnFailure(OnUnsolicitedReportData(apExchangeContext, aPayloadHeader, std::move(aPayload)));
status = Protocols::InteractionModel::Status::Success;
status = Status::Success;
}
else if (aPayloadHeader.HasMessageType(MsgType::TimedRequest))
{
Expand All @@ -579,7 +579,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
ChipLogProgress(InteractionModel, "Msg type %d not supported", aPayloadHeader.GetMessageType());
}

if (status != Protocols::InteractionModel::Status::Success && !apExchangeContext->IsGroupExchangeContext())
if (status != Status::Success && !apExchangeContext->IsGroupExchangeContext())
{
return StatusResponse::Send(status, apExchangeContext, false /*aExpectResponse*/);
}
Expand Down Expand Up @@ -614,6 +614,8 @@ bool InteractionModelEngine::TrimFabricForSubscriptions(FabricIndex aFabricIndex
return false;
}

// Note: This is OK only when we have assumed the fabricCount is not zero. Should be revised when adding support to
// subscriptions on PASE sessions.
size_t perFabricPathCapacity = pathPoolCapacity / static_cast<size_t>(fabricCount);
size_t perFabricSubscriptionCapacity = readHandlerPoolCapacity / static_cast<size_t>(fabricCount);

Expand Down Expand Up @@ -821,7 +823,7 @@ bool InteractionModelEngine::TrimFabricForRead(FabricIndex aFabricIndex)
{
candidate = handler;
}
// This handler is younger than the one we picked before, should evict the younger one.
// Read Handlers are "first come first served", so we give eariler read transactions a higher priority.
else if (handler->GetTransactionStartGeneration() > candidate->GetTransactionStartGeneration() &&
// And the level of resource usage is the same (both exceed or neither exceed)
((attributePathsUsed > kMinSupportedPathsPerReadRequest || eventPathsUsed > kMinSupportedPathsPerReadRequest) ==
Expand Down Expand Up @@ -873,6 +875,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::EnsureResourceForRea
const size_t readHandlerCap = allowUnlimited ? SIZE_MAX : GetReadHandlerPoolCapacityForReads();

const size_t guaranteedReadRequestsPerFabric = GetGuaranteedReadRequestsPerFabric();
const size_t guaranteedPathsPerFabric = kMinSupportedPathsPerReadRequest * guaranteedReadRequestsPerFabric;

size_t usedAttributePaths = 0;
size_t usedEventPaths = 0;
Expand All @@ -894,28 +897,32 @@ Protocols::InteractionModel::Status InteractionModelEngine::EnsureResourceForRea
});
};

auto haveEnoughResourcesForTheRequest = [&]() {
return usedAttributePaths + aRequestedAttributePathCount <= attributePathCap &&
usedEventPaths + aRequestedEventPathCount <= eventPathCap && usedReadHandlers < readHandlerCap;
};

countResourceUsage();

if (usedAttributePaths + aRequestedAttributePathCount <= attributePathCap &&
usedEventPaths + aRequestedEventPathCount <= eventPathCap && usedReadHandlers < readHandlerCap)
if (haveEnoughResourcesForTheRequest())
{
// We have enough resources, then we serve the requests in a best-effort manner.
return Protocols::InteractionModel::Status::Success;
return Status::Success;
}

if ((aRequestedAttributePathCount > kMinSupportedPathsPerReadRequest &&
usedAttributePaths + aRequestedAttributePathCount > attributePathCap) ||
(aRequestedEventPathCount > kMinSupportedPathsPerReadRequest && usedEventPaths + aRequestedEventPathCount > eventPathCap))
{
// We cannot offer enough resources, and the read transaction is requesting more than the spec limit.
return Protocols::InteractionModel::Status::PathsExhausted;
return Status::PathsExhausted;
}

// If we have commissioned CHIP_CONFIG_MAX_FABRICS already, and this transaction don't have an associated fabric index, reject
// If we have commissioned CHIP_CONFIG_MAX_FABRICS already, and this transaction doesn't have an associated fabric index, reject
// the request if we don't have sufficient resources for this request.
if (mpFabricTable->FabricCount() == GetConfigMaxFabrics() && aFabricIndex == kUndefinedFabricIndex)
{
return Protocols::InteractionModel::Status::ResourceExhausted;
return Status::Busy;
}

size_t usedAttributePathsInFabric = 0;
Expand All @@ -932,13 +939,12 @@ Protocols::InteractionModel::Status InteractionModelEngine::EnsureResourceForRea
return Loop::Continue;
});

// Busy, since there is already some read requests ongoing on this fabric, please retry later.
if (usedAttributePathsInFabric + aRequestedAttributePathCount >
kMinSupportedPathsPerReadRequest * guaranteedReadRequestsPerFabric ||
usedEventPathsInFabric + aRequestedEventPathCount > kMinSupportedPathsPerReadRequest * guaranteedReadRequestsPerFabric ||
// Busy, since there are already some read requests ongoing on this fabric, please retry later.
if (usedAttributePathsInFabric + aRequestedAttributePathCount > guaranteedPathsPerFabric ||
usedEventPathsInFabric + aRequestedEventPathCount > guaranteedPathsPerFabric ||
usedReadHandlersInFabric >= guaranteedReadRequestsPerFabric)
{
return Protocols::InteractionModel::Status::Busy;
return Status::Busy;
}

const auto evictAndUpdateResourceUsage = [&](FabricIndex fabricIndex) {
Expand All @@ -957,8 +963,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::EnsureResourceForRea
{
didEvictHandler = false;
didEvictHandler = didEvictHandler || evictAndUpdateResourceUsage(kUndefinedFabricIndex);
if (usedAttributePaths + aRequestedAttributePathCount <= attributePathCap &&
usedEventPaths + aRequestedEventPathCount <= eventPathCap && usedReadHandlers < readHandlerCap)
if (haveEnoughResourcesForTheRequest())
{
break;
}
Expand All @@ -970,29 +975,20 @@ Protocols::InteractionModel::Status InteractionModelEngine::EnsureResourceForRea
}
for (const auto & fabric : *mpFabricTable)
{
didEvictHandler = didEvictHandler || evictAndUpdateResourceUsage(fabric.GetFabricIndex());
// The resources are enough to serve this request, do not evict anything.
if (usedAttributePaths + aRequestedAttributePathCount <= attributePathCap &&
usedEventPaths + aRequestedEventPathCount <= eventPathCap && usedReadHandlers < readHandlerCap)
if (haveEnoughResourcesForTheRequest())
{
break;
}
didEvictHandler = didEvictHandler || evictAndUpdateResourceUsage(fabric.GetFabricIndex());
}
}

// The read requests that are not using exceeded resources will be first-come-first-served, since the read transactions will be
// relatively short, the client is expected to retry later if the server is busy now.
if (usedAttributePaths + aRequestedAttributePathCount > attributePathCap ||
usedEventPaths + aRequestedEventPathCount > eventPathCap)
{
return Protocols::InteractionModel::Status::Busy;
}
if (usedReadHandlers >= readHandlerCap)
{
return Protocols::InteractionModel::Status::Busy;
}
// Now all fabrics are not oversized (since we have trimmed the oversized fabrics in the loop above), and the read handler is
// also not oversized, we should be able to handle this read transaction.
VerifyOrDie(haveEnoughResourcesForTheRequest());

return Protocols::InteractionModel::Status::Success;
return Status::Success;
}

void InteractionModelEngine::RemoveReadClient(ReadClient * apReadClient)
Expand Down
23 changes: 13 additions & 10 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,11 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
bool TrimFabricForSubscriptions(FabricIndex aFabricIndex, bool aForceEvict);

/**
* Select the oldest (and the one that exceeds the per read resource minimum if there are any) read handler on the fabric with
* the given fabric index.
* Select a read handler and abort the read transaction if the fabric is using more resources (number of paths or number of read
* handlers) then we guaranteed.
*
* - The youngest oversized read handlers will be chosen first.
* - If there are no oversized read handlers, the youngest read handlers will be chosen.
*
* @retval Whether we have evicted a read transaction.
*/
Expand Down Expand Up @@ -470,20 +473,20 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
* - If the existing resources can serve this read transaction, this function will return Status::Success.
* - or if the resources used by read transactions in the fabric index meets the per fabric resource limit (i.e. 9 paths & 1
* read) after accepting this read request, this function will always return Status::Success by evicting existing read
* transactions which uses more resources than kMinSupportedPathsPerReadRequest from the oldest to the newest.
* transactions from other fabrics which are using more than the guaranteed minimum number of read.
* - or if the resources used by read transactions in the fabric index will exceed the per fabric resource limit (i.e. 9 paths &
* 1 read) after accepting this read request, this function will return false without evicting any existing transaction.
* 1 read) after accepting this read request, this function will return a failure status without evicting any existing
* transaction.
* - However, read transactions on PASE sessions won't evict any existing read transactions when we have already commissioned
* CHIP_CONFIG_MAX_FABRICS fabrics on the device.
*
* @retval Status::Success: The read transaction can be accepted.
* @retval Status::Busy: The read handler pool is exhausted. But the client are expected to retry later.
* @retval Status::ResourceExhausted: The read handler pool is exhausted. But the client are not expected to retry later (PASE
* session with a full fabric table).
* @retval Status::PathsExhausted: The attribute / event path pool is exhausted.
* @retval Status::Busy: The remaining resource is insufficient to handle this read request, and the accessing fabric for this
* read request will use more resources than we guaranteed, the client is expected to retry later.
* @retval Status::PathsExhausted: The attribute / event path pool is exhausted, and the read request is requesting more
* resources than we guaranteed.
*/
Protocols::InteractionModel::Status EnsureResourceForRead(FabricIndex aFabricIndex, size_t aRequestedAttributePathCount,
size_t aRequestedEventPathCount);
Status EnsureResourceForRead(FabricIndex aFabricIndex, size_t aRequestedAttributePathCount, size_t aRequestedEventPathCount);

template <typename T, size_t N>
void ReleasePool(ObjectList<T> *& aObjectList, ObjectPool<ObjectList<T>, N> & aObjectPool);
Expand Down
4 changes: 2 additions & 2 deletions src/controller/tests/data_model/TestRead.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3637,7 +3637,7 @@ void TestReadInteraction::TestReadHandler_ParallelReads(nlTestSuite * apSuite, v
});

// Case 6: The device's fabric table is full, PASE sessions won't be counted as a valid fabric and cannot evict existing read
// transactions -> If we don't have enough resources, it will receive ResourceExhausted.
// transactions. It will be rejected with Busy status code.
TestCase(
Params{
.ReadHandlerCapacity = 3,
Expand Down Expand Up @@ -3681,7 +3681,7 @@ void TestReadInteraction::TestReadHandler_ParallelReads(nlTestSuite * apSuite, v

// The new read request should be rejected.
NL_TEST_ASSERT(apSuite, readCallback.mOnError == 1);
NL_TEST_ASSERT(apSuite, readCallback.mLastError == CHIP_IM_GLOBAL_STATUS(ResourceExhausted));
NL_TEST_ASSERT(apSuite, readCallback.mLastError == CHIP_IM_GLOBAL_STATUS(Busy));
// Should evict one read request from Bob fabric for enough resources.
NL_TEST_ASSERT(apSuite,
app::InteractionModelEngine::GetInstance()->GetNumActiveReadHandlers(
Expand Down
12 changes: 3 additions & 9 deletions src/lib/core/CHIPConfig.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -821,12 +821,9 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
#endif

/**
* @def CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS
* @def CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS_FOR_SUBSCRIPTIONS
*
* @brief Defines the maximum number of path objects, limits the number of attributes being read or subscribed at the same time.
*
* The default value comes from 3path per subsctipion * 3sub per fabric * max number of fabrics, then reserve 1 read client with 9
* paths for each fabric.
* @brief The maximum number of path objects for subscriptions, limits the number of attributes being subscribed at the same time.
*/
#ifndef CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS_FOR_SUBSCRIPTIONS
// #define CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS (CHIP_IM_MAX_NUM_SUBSCRIPTIONS * 3)
Expand All @@ -837,10 +834,7 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
/**
* @def CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS_FOR_READS
*
* @brief Defines the maximum number of path objects, limits the number of attributes being read or subscribed at the same time.
*
* The default value comes from 3path per subsctipion * 3sub per fabric * max number of fabrics, then reserve 1 read client with 9
* paths for each fabric.
* @brief Defines the maximum number of path objects for read requests.
*/
#ifndef CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS_FOR_READS
#define CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS_FOR_READS (CHIP_IM_MAX_NUM_READS * 9)
Expand Down

0 comments on commit fbeca4c

Please sign in to comment.