Skip to content

Commit

Permalink
Improve size calculation for our packets. (#28563)
Browse files Browse the repository at this point in the history 
* Improve size calculation for our packets.

We were hardcoding kMaxAppMessageLen and kMaxSecureSduLengthBytes, but those
defines were not even consistent with each other, and both were overly
conservative.  Specific changes:

1. Fix computation of CHIP_SYSTEM_HEADER_RESERVE_SIZE: we don't have any "crypto
   headers".
2. Fix computation of kMaxAppMessageLen to use our actual packet buffer sizes
   and defined header sizes.
3. Fix kMaxSecureSduLengthBytes to be consistent with kMaxAppMessageLen.

* Simplify the size computation logic.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Apr 12, 2024
1 parent 68a57e9 commit 2275845
Show file tree
Hide file tree
Showing 7 changed files with 128 additions and 79 deletions.
3 changes: 2 additions & 1 deletion src/app/StatusResponse.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@
#include <messaging/ExchangeContext.h>
#include <protocols/interaction_model/Constants.h>
#include <system/TLVPacketBufferBackingStore.h>
#include <transport/raw/MessageHeader.h>

namespace chip {
namespace app {
static constexpr size_t kMaxSecureSduLengthBytes = 1024;
static constexpr size_t kMaxSecureSduLengthBytes = kMaxAppMessageLen + kMaxTagLen;

class StatusResponse
{
Expand Down
55 changes: 30 additions & 25 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ chip::EndpointId kInvalidTestEndpointId = 3;
chip::DataVersion kTestDataVersion1 = 3;
chip::DataVersion kTestDataVersion2 = 5;

// Number of items in the list for MockAttributeId(4).
constexpr int kMockAttribute4ListLength = 6;

static chip::System::Clock::Internal::MockClock gMockClock;
static chip::System::Clock::ClockBase * gRealClock;

Expand Down Expand Up @@ -1214,7 +1217,8 @@ void TestReadInteraction::TestReadChunking(nlTestSuite * apSuite, void * apConte
NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);

chip::app::AttributePathParams attributePathParams[1];
// Mock Attribute 4 is a big attribute, with 6 large OCTET_STRING
// Mock Attribute 4 is a big attribute, with kMockAttribute4ListLength large
// OCTET_STRING elements.
attributePathParams[0].mEndpointId = Test::kMockEndpoint3;
attributePathParams[0].mClusterId = Test::MockClusterId(2);
attributePathParams[0].mAttributeId = Test::MockAttributeId(4);
Expand All @@ -1234,8 +1238,10 @@ void TestReadInteraction::TestReadChunking(nlTestSuite * apSuite, void * apConte

ctx.DrainAndServiceIO();

// We get one chunk with 3 array elements, and then one chunk per element.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 4);
// We get one chunk with 4 array elements, and then one chunk per
// element, and the total size of the array is
// kMockAttribute4ListLength.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 1 + (kMockAttribute4ListLength - 4));
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 6);
NL_TEST_ASSERT(apSuite, delegate.mGotReport);
NL_TEST_ASSERT(apSuite, !delegate.mReadError);
Expand Down Expand Up @@ -1380,13 +1386,16 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void

ctx.DrainAndServiceIO();

// We should receive another (3 + 1) = 4 attribute reports represeting 6
// array items, since the underlying path iterator should be reset to
// the beginning of the cluster it is currently iterating.
// Our list has length kMockAttribute4ListLength. Since the underlying
// path iterator should be reset to the beginning of the cluster it is
// currently iterating, we expect to get another value for our
// attribute. The way the packet boundaries happen to fall, that value
// will encode 4 items in the first IB and then one IB per item.
const int expectedIBs = 1 + (kMockAttribute4ListLength - 4);
ChipLogError(DataManagement, "OLD: %d\n", currentAttributeResponsesWhenSetDirty);
ChipLogError(DataManagement, "NEW: %d\n", delegate.mNumAttributeResponse);
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + 4);
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == currentArrayItemsWhenSetDirty + 6);
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + expectedIBs);
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == currentArrayItemsWhenSetDirty + kMockAttribute4ListLength);
NL_TEST_ASSERT(apSuite, delegate.mGotReport);
NL_TEST_ASSERT(apSuite, !delegate.mReadError);
// By now we should have closed all exchanges and sent all pending acks, so
Expand Down Expand Up @@ -2341,7 +2350,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
// paths, for a total of 58 attributes.
//
// Attribute 0xFFFC::0xFFF1'FC02::0xFFF1'0004 (kMockEndpoint3::MockClusterId(2)::MockAttributeId(4))
// is a list of 6 elements of size 256 bytes each, which cannot fit in a single
// is a list of kMockAttribute4ListLength elements of size 256 bytes each, which cannot fit in a single
// packet, so gets list chunking applied to it.
//
// Because delegate.mNumAttributeResponse counts AttributeDataIB instances, not attributes,
Expand All @@ -2350,21 +2359,16 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
// in the response, there will be one AttributeDataIB for the start of the list (which will include
// some number of 256-byte elements), then one AttributeDataIB for each of the remaining elements.
//
// When EventList is enabled, for the first report for the list attribute we receive two
// of its items in the initial list, then 4 additional items. For the second report we
// receive 3 items in the initial list followed by 3 additional items.
//
// Thus we should receive 29*2 + 4 + 3 = 65 attribute data in total.
constexpr size_t kExpectedAttributeResponse = 65;
// When EventList is enabled, for the first report for the list attribute we receive three
// of its items in the initial list, then the remaining items. For the second report we
// receive 2 items in the initial list followed by the remaining items.
constexpr size_t kExpectedAttributeResponse = 29 * 2 + (kMockAttribute4ListLength - 3) + (kMockAttribute4ListLength - 2);
#else
// When EventList is not enabled, the packet boundaries shift and for the first
// report for the list attribute we receive two of its items in the initial list,
// then 4 additional items. For the second report we receive 3 items in
// the initial list followed by 3 additional items.
//
// Thus we should receive 29*2 + 4 + 3 = 65 attribute data when the eventlist
// attribute is not available.
constexpr size_t kExpectedAttributeResponse = 65;
// report for the list attribute we receive four of its items in the initial list,
// then additional items. For the second report we receive 4 items in
// the initial list followed by additional items.
constexpr size_t kExpectedAttributeResponse = 29 * 2 + (kMockAttribute4ListLength - 4) + (kMockAttribute4ListLength - 4);
#endif
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == kExpectedAttributeResponse);
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 12);
Expand Down Expand Up @@ -3460,9 +3464,10 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReport(nlTestSuite * ap
gMockClock.AdvanceMonotonic(System::Clock::Milliseconds32(10));
}
}
// Two chunked reports carry 4 attributeDataIB: 1 with a list of 3 items,
// and then one per remaining item.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 4);
// We get one chunk with 4 array elements, and then one chunk per
// element, and the total size of the array is
// kMockAttribute4ListLength.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 1 + (kMockAttribute4ListLength - 4));
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 6);

NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);
Expand Down
6 changes: 4 additions & 2 deletions src/app/tests/TestWriteInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,8 +579,9 @@ void TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage(nlTestSuite * a
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), app::reporting::GetDefaultReportScheduler());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

// Reserve all except the last 128 bytes, so that we make sure to chunk.
app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* reserved buffer size */);
static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);

ByteSpan list[5];

Expand Down Expand Up @@ -647,8 +648,9 @@ void TestWriteInteraction::TestWriteHandlerInvalidateFabric(nlTestSuite * apSuit
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), app::reporting::GetDefaultReportScheduler());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

// Reserve all except the last 128 bytes, so that we make sure to chunk.
app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* reserved buffer size */);
static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);

ByteSpan list[5];

Expand Down
38 changes: 21 additions & 17 deletions src/controller/tests/TestWriteChunking.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,21 +203,22 @@ void TestWriteChunking::TestListChunking(nlTestSuite * apSuite, void * apContext

app::AttributePathParams attributePath(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);
//
// We've empirically determined that by reserving 950 bytes in the packet buffer, we can fit 2
// We've empirically determined that by reserving all but 75 bytes in the packet buffer, we can fit 2
// AttributeDataIBs into the packet. ~30-40 bytes covers a single write chunk, but let's 2-3x that
// to ensure we'll sweep from fitting 2 chunks to 3-4 chunks.
//
for (int i = 100; i > 0; i--)
constexpr size_t minReservationSize = kMaxSecureSduLengthBytes - 75 - 100;
for (uint32_t i = 100; i > 0; i--)
{
CHIP_ERROR err = CHIP_NO_ERROR;
TestWriteCallback writeCallback;

ChipLogDetail(DataManagement, "Running iteration %d\n", i);

gIterationCount = (uint32_t) i;
gIterationCount = i;

app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(850 + i) /* reserved buffer size */);
static_cast<uint16_t>(minReservationSize + i) /* reserved buffer size */);

ByteSpan list[kTestListLength];

Expand Down Expand Up @@ -358,15 +359,16 @@ void TestWriteChunking::TestConflictWrite(nlTestSuite * apSuite, void * apContex

app::AttributePathParams attributePath(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);

/* use a smaller chunk (128 bytes) so we only need a few attributes in the write request. */
constexpr size_t kReserveSize = kMaxSecureSduLengthBytes - 128;

TestWriteCallback writeCallback1;
app::WriteClient writeClient1(
&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient1(&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

TestWriteCallback writeCallback2;
app::WriteClient writeClient2(
&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient2(&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

ByteSpan list[kTestListLength];

Expand Down Expand Up @@ -435,15 +437,16 @@ void TestWriteChunking::TestNonConflictWrite(nlTestSuite * apSuite, void * apCon
app::AttributePathParams attributePath1(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);
app::AttributePathParams attributePath2(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute2);

/* use a smaller chunk (128 bytes) so we only need a few attributes in the write request. */
constexpr size_t kReserveSize = kMaxSecureSduLengthBytes - 128;

TestWriteCallback writeCallback1;
app::WriteClient writeClient1(
&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient1(&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

TestWriteCallback writeCallback2;
app::WriteClient writeClient2(
&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient2(&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

ByteSpan list[kTestListLength];

Expand Down Expand Up @@ -514,7 +517,8 @@ void RunTest(nlTestSuite * apSuite, TestContext & ctx, Instructions instructions
TestWriteCallback writeCallback;
std::unique_ptr<WriteClient> writeClient = std::make_unique<WriteClient>(
&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
static_cast<uint16_t>(kMaxSecureSduLengthBytes -
128) /* use a smaller chunk so we only need a few attributes in the write request. */);

ConcreteAttributePath onGoingPath = ConcreteAttributePath();
std::vector<PathStatus> status;
Expand Down
22 changes: 15 additions & 7 deletions src/controller/tests/data_model/TestRead.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1063,7 +1063,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
}

// Read of E2C3A* and E3C2A2, and inject a large amount of event path list, then it would try to apply previous cache
// latest data version and construct data version list but no enough memory, finally fully rollback data version filter. Expect
// latest data version and construct data version list but run out of memory, finally fully rollback data version filter. Expect
// E2C3A* attributes in report, and E3C2A2 attribute in report
{
testId++;
Expand All @@ -1081,8 +1081,12 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
readPrepareParams.mpAttributePathParamsList = attributePathParams2;
readPrepareParams.mAttributePathParamsListSize = 2;

readPrepareParams.mpEventPathParamsList = eventPathParams;
readPrepareParams.mEventPathParamsListSize = 64;
readPrepareParams.mpEventPathParamsList = eventPathParams;
// This size needs to be big enough that we can't fit our
// DataVersionFilterIBs in the same packet. Max size is
// ArraySize(eventPathParams);
static_assert(75 <= ArraySize(eventPathParams));
readPrepareParams.mEventPathParamsListSize = 75;

err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand Down Expand Up @@ -1252,8 +1256,8 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit

// Read of E1C2A*(3 attributes) and E2C3A*(5 attributes) and E2C2A*(4 attributes), and inject a large amount of event path
// list, then it would try to apply previous cache latest data version and construct data version list with the ordering from
// largest cluster size to smallest cluster size(C2, C3, C1) but no enough memory, finally partially rollback data version
// filter with only C2. Expect E1C2A*, E2C2A* attributes(7 attributes) in report,
// largest cluster size to smallest cluster size(C3, C2, C1) but run out of memory, finally partially rollback data version
// filter with only C3. Expect E1C2A*, E2C2A* attributes(7 attributes) in report,
{
testId++;
ChipLogProgress(DataManagement, "\t -- Running Read with ClusterStateCache Test ID %d", testId);
Expand All @@ -1275,8 +1279,12 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
readPrepareParams.mpAttributePathParamsList = attributePathParams3;
readPrepareParams.mAttributePathParamsListSize = 3;
readPrepareParams.mpEventPathParamsList = eventPathParams;
readPrepareParams.mEventPathParamsListSize = 62;
err = readClient.SendRequest(readPrepareParams);

// This size needs to be big enough that we can only fit our first
// DataVersionFilterIB. Max size is ArraySize(eventPathParams);
static_assert(73 <= ArraySize(eventPathParams));
readPrepareParams.mEventPathParamsListSize = 73;
err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();
Expand Down
54 changes: 28 additions & 26 deletions src/system/SystemConfig.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,55 +293,57 @@
#endif // CHIP_SYSTEM_CONFIG_ZEPHYR_LOCKING && CHIP_SYSTEM_CONFIG_NO_LOCKING

/**
* @def CHIP_SYSTEM_HEADER_RESERVE_SIZE
* @def CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE
*
* @brief
* The number of bytes to reserve in a network packet buffer to contain
* the CHIP message and exchange headers.
*
* This number was calculated as follows:
* the Matter crypto headers.
*
* CHIP Crypto Header:
*
* 4 -- Length of encrypted block
* 4 -- Reserve
* 8 -- Initialization Vector
* 8 -- Encryption Tag
* This is 0, because Matter does not have any crypto headers. This define
* is still here only for backwards compatibility.
*/
#ifndef CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE
#define CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE 24
#define CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE 0
#endif

/**
* @def CHIP_SYSTEM_HEADER_RESERVE_SIZE
*
* @brief
* The number of bytes to reserve in a network packet buffer to contain
* the CHIP message and exchange headers.
* the CHIP message and payload headers.
*
* This number was calculated as follows:
*
* CHIP Message Header:
* Matter Message Header:
*
* 2 -- Frame Length
* 2 -- Message Header
* 4 -- Message Id
* 8 -- Source Node Id
* 8 -- Destination Node Id
* 2 -- Key Id
* 1 -- Message Flags
* 2 -- Session ID
* 1 -- Security Flags
* 4 -- Message Counter
* 8 -- Source Node ID
* 8 -- Destination Node ID
*
* Total: 26 bytes.
*
* Matter Payload Header:
*
* CHIP Exchange Header:
* 1 -- Exhange Flags
* 1 -- Protocol Opcode
* 2 -- Exchange ID
* 2 -- Protocol Vendor ID
* 2 -- Protocol ID
* 4 -- Acknowledged MEssage Counter
*
* 1 -- Application Version
* 1 -- Message Type
* 2 -- Exchange Id
* 4 -- Profile Id
* 4 -- Acknowledged Message Id
* Total: 12 bytes.
*
* @note A number of these fields are optional or not presently used. So most headers will be considerably smaller than this.
* @note A number of these fields are optional or not presently used. So most
* headers will be considerably smaller than this.
* @note This calculation assumes ther are no Message Extensions or Secured Extensions.
*/
#ifndef CHIP_SYSTEM_HEADER_RESERVE_SIZE
#define CHIP_SYSTEM_HEADER_RESERVE_SIZE (38 + CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE)
#define CHIP_SYSTEM_HEADER_RESERVE_SIZE (26 + 12 + CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE)
#endif /* CHIP_SYSTEM_HEADER_RESERVE_SIZE */

/**
Expand Down
Loading

0 comments on commit 2275845

Please sign in to comment.