Changes for large Packetbuffer allocation to support TCP payloads #33308
+141
−74
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PR #33308: Size comparison from 062e063 to 6dd1c8e Increases (32 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, linux)
Decreases (12 builds for cc13x4_26x4, cc32xx, efr32, linux, psoc6)
Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
|
There was a problem hiding this comment.
I have a general question before finishing the review, how are you going to support allocation of large messages on platforms that use buffer pools?
I wonder if we should support configurations in which normal messages are allocated from pools but large messages are allocated on the heap. In other words, I'm curious if having a single PacketBufferHandle::New() method for allocating both normal and large messages is a good and sufficient way forward.
@Damian-Nordic, platforms that use buffer pools and require TCP need to adapt the implementation towards that. The current model of supporting large messages uses a heap-based allocation scheme and the choice of a heap-based model is done at compile-time. At the moment it seemed an added complexity to make the heap based allocation for TCP dynamic, and allow regular messages on such platforms to use buffer pools. What is the issue in using heaps for both regular and large messages on TCP-enabled systems? |
Buffer (and other) pools are used in embedded devices to secure memory for the most critical functionalities and reduce the heap fragmentation so I was just curious about the plan. |
|
@pidarped it seems NRF builds complain about |
|
@pidarped please apply restyle fixes, otherwise the bots will not add reviewers. |
andy31415
reviewed
May 7, 2024
andy31415
reviewed
May 7, 2024
andy31415
reviewed
May 7, 2024
bzbarsky-apple
requested changes
May 7, 2024
src/system/SystemPacketBuffer.cpp
Outdated
| @@ -530,12 +530,19 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese | |||
|
|
|||
| CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle()); | |||
|
|
|||
| // TODO: Change the max to a lower value | |||
| if (aAvailableSize > UINT32_MAX || lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT32_MAX) | |||
| #if INET_CONFIG_ENABLE_TCP_ENDPOINT | |||
There was a problem hiding this comment.
Also, its seems weird to condition this on INET_CONFIG_ENABLE_TCP_ENDPOINT. Seems to me like we should have CHIP_CONFIG_MAX_LARGE_PAYLOAD_SIZE_BYTES as a value. That value might be 0 (or might be something very large?) if large payloads are not supported, and then we can have non-ifdefed code. I guess this goes to @andy31415 's comment about the logic duplication here too. What's here right now seems really fragile and is very hard to read, which is a huge red flag for security-critical size checks.
There was a problem hiding this comment.
Defined constants in SystemPacketBuffer.h for large payload values that have been used in some of the static_asserts here.
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
6dd1c8e
to
39cfea4
Compare
|
PR #33308: Size comparison from 1b455b5 to 39cfea4 Decreases (2 builds for mbed, stm32)
Full report (2 builds for mbed, stm32)
|
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
2 times, most recently
from
61a4a0d
to
900828c
Compare
|
PR #33308: Size comparison from ef68373 to 900828c Increases (15 builds for linux)
Decreases (31 builds for esp32, linux, mbed, nxp, qpg, stm32, telink)
Full report (43 builds for esp32, linux, mbed, nxp, qpg, stm32, telink)
|
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
900828c
to
094aadd
Compare
|
PR #33308: Size comparison from 7b2f729 to 094aadd Increases (17 builds for linux, nxp)
Decreases (24 builds for esp32, linux, nxp, qpg, stm32, telink)
Full report (43 builds for esp32, linux, mbed, nxp, qpg, stm32, telink)
|
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
094aadd
to
cbe8b8f
Compare
|
PR #33308: Size comparison from fb8d9e5 to cbe8b8f Increases (17 builds for linux, nxp)
Decreases (24 builds for esp32, linux, nxp, qpg, stm32, telink)
Full report (43 builds for esp32, linux, mbed, nxp, qpg, stm32, telink)
|
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
cbe8b8f
to
4042de3
Compare
|
PR #33308: Size comparison from b398fb4 to 4042de3 Increases (22 builds for cc13x4_26x4, linux, nxp)
Decreases (54 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink)
Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
|
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
4042de3
to
01834db
Compare
|
PR #33308: Size comparison from 3718e99 to 01834db Increases (23 builds for cc13x4_26x4, linux, nxp)
Decreases (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
|
|
Doesn't this break backwards compatibility? Or was nobody using TCP with 16-bit message lengths in production yet? |
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
01834db
to
fa93f3d
Compare
|
PR #33308: Size comparison from 3718e99 to fa93f3d Increases (7 builds for cc13x4_26x4, linux)
Decreases (9 builds for cc13x4_26x4, cc32xx, linux)
Full report (9 builds for cc13x4_26x4, cc32xx, linux)
|
@nicolas17, yes that is right. TCP was not being used in production yet which gave us the opportunity to increase the message length field to 4 bytes to include large messages. |
pullapprove
bot
requested review from
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #33308: Size comparison from 3718e99 to f593351 Increases (23 builds for cc13x4_26x4, linux, nxp)
Decreases (77 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
|
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
f593351
to
c0b2693
Compare
|
PR #33308: Size comparison from 22a9dc3 to c0b2693 Increases (23 builds for bl702l, cc13x4_26x4, linux, nrfconnect)
Decreases (91 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Full report (94 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
andy31415
reviewed
May 30, 2024
| @@ -129,7 +129,7 @@ matter_add_gn_arg_bool ("chip_enable_nfc" CONFIG_CHIP_NF | |||
| matter_add_gn_arg_bool ("chip_enable_ota_requestor" CONFIG_CHIP_OTA_REQUESTOR) | |||
| matter_add_gn_arg_bool ("chip_persist_subscriptions" CONFIG_CHIP_PERSISTENT_SUBSCRIPTIONS) | |||
| matter_add_gn_arg_bool ("chip_monolithic_tests" CONFIG_CHIP_BUILD_TESTS) | |||
| matter_add_gn_arg_bool ("chip_inet_config_enable_tcp_endpoint" CONFIG_CHIP_BUILD_TESTS) | |||
| matter_add_gn_arg_bool ("chip_inet_config_enable_tcp_endpoint" FALSE) | |||
There was a problem hiding this comment.
This change should be explained in the PR summary: why did we change this from generally "yes for tests" to "never". Describe why this is a better path than removing the test for NRF (e.g. splitting tests into 2 and have one of the cpp files not included for nrf)
There was a problem hiding this comment.
Added some justification in the PR summary. A future approach would be to split the file up but for now, since the platform is not using TCP, it is disabled.
andy31415
reviewed
May 30, 2024
| @@ -522,7 +522,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint> | |||
| /** | |||
| * Size of the largest TCP packet that can be received. | |||
| */ | |||
| constexpr static size_t kMaxReceiveMessageSize = System::PacketBuffer::kMaxSizeWithoutReserve; | |||
| constexpr static size_t kMaxReceiveMessageSize = System::PacketBuffer::kMaxAllocSize; | |||
There was a problem hiding this comment.
Could you also explain this change? I believe that the "withoutreserve" vs maxsize is a bit messy, however overall this seems to change a constant with something else, so it is a change in existing behaviourt even for UDP. I would like to make sure it is correct.
There was a problem hiding this comment.
kMaxAllocSize is a unified name for the MaxSizeWithoutReserve for both regular and large buffers. It is defined in SystemPacketBuffer.h. Here for TCP, kMaxAllocSize essentially is kLargeBufMaxSizeWithoutReserve.
andy31415
reviewed
May 30, 2024
andy31415
reviewed
May 30, 2024
src/system/SystemPacketBuffer.cpp
Outdated
| "Max size for Large payload buffers cannot exceed UINT32_MAX"); | ||
| #endif // CHIP_SYSTEM_CONFIG_USE_LWIP | ||
|
|
||
| // Ensure that the definition of the max buffer allocation for regular |
There was a problem hiding this comment.
This seems to re-read the code. Could you update it to say why this is needed? Why is this a sctrict inequality rather than "<="? What does the extra 1 byte give us?
There was a problem hiding this comment.
Actually, this static_assert may not be necessary after all. If a system defines the large buffer max size really low(for whatever reason) we do not need to prevent it from using the TCP transport.
andy31415
reviewed
May 30, 2024
andy31415
reviewed
May 30, 2024
| @@ -557,7 +574,7 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese | |||
|
|
|||
| CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle()); | |||
|
|
|||
| if (lAllocSize > PacketBuffer::kMaxSizeWithoutReserve) | |||
| if (lAllocSize > PacketBuffer::kMaxAllocSize) | |||
There was a problem hiding this comment.
why this change? Did we update meanings for some constants?
There was a problem hiding this comment.
In order to avoid ifdef'ing here, kMaxAllocSize is the unified constant for kMaxSizeWithoutReserve for both regular and large buffers(defined in SystemPacketBuffer.h). The name choice comes from here where it is an upper bound on the AllocSize as depicted in https://github.com/project-chip/connectedhomeip/blob/master/src/system/SystemPacketBuffer.h#L94.
andy31415
reviewed
May 30, 2024
andy31415
reviewed
May 30, 2024
andy31415
reviewed
May 30, 2024
| #if INET_CONFIG_ENABLE_TCP_ENDPOINT | ||
| static constexpr size_t kMaxAllocSize = kLargeBufMaxSizeWithoutReserve; | ||
| #else | ||
| static constexpr size_t kMaxAllocSize = kMaxSizeWithoutReserve; |
There was a problem hiding this comment.
I find this confusing: we still have kMaxSizeWithoutReserve however then kMaxSizeWithoutReserve is not used much except in some asserts and in computing these things. Why do we keep the asserts then?
These sizes need some form of diagram/explanation. I do not know what a reserve is and I do not understand the difference between "large buffer" vs other.
https://github.com/project-chip/connectedhomeip/blob/master/src/system/SystemPacketBuffer.h#L94 has a diagram in terms of Member() calls ... can we have something similar explaining these constants, probably closer to this area? I cannot seem to just read the PR and see that everything is fine and because of the renaming of kMaxSizeWithoutReserve to kMaxAllocSize I worry that we may change behavior in unintended ways.
There was a problem hiding this comment.
kMaxSizeWithoutReserve is used as the max allocation size for MRP based logic at several places, including Test code. The reserve space is to contain the encapsulation header fields(Matter message and exchange header). It is defined in detail in SystemConfig.h
The renaming to kMaxAllocSize as a unified constant actually aligns with the diagram in https://github.com/project-chip/connectedhomeip/blob/master/src/system/SystemPacketBuffer.h#L94, which is why I chose it. It is, essentially, the max of AllocSize() that encompasses both Reserve and Data space in the buffer.
Changes to internal checks in SystemPacketBuffer. Update the length encoding for TCP payloads during send and receive. Max size config for large packetbuffer size limit in SystemPacketBuffer.h. Define Max application payload size for large buffers Test modifications for chainedbuffer receives for TCP. - Add test for Buffer length greater than MRP max size. - Disable TCP on nrfconnect platform because of resource requirements. Stack allocations for large buffer with default size is exceeding limits. Disabling the Test file altogether for this platform would prevent all tests from running. Instead, only disabling TCP on nrfConnect for now, since it is unused. Fixes Issues project-chip#31779, project-chip#33307.
pidarped
force-pushed
the
PacketBufferLargeChecks
branch
from
c0b2693
to
bfa802e
Compare
|
PR #33308: Size comparison from b790232 to bfa802e Increases (17 builds for linux, nrfconnect)
Decreases (90 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Full report (94 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes to internal checks in SystemPacketBuffer.
Update the length encoding for TCP payloads during send and receive.
Max size config for large packetbuffer size limit in SystemPacketBuffer.h.
Test modifications for chainedbuffer receives for TCP.
- Add test for Buffer length greater than MRP max size.
Disable TCP on nrfconnect platform because of resource requirements.
Stack allocations for large buffer with default size is exceeding
limits. Disabling the Test file altogether for this platform would
prevent all tests from running. Instead, only disabling TCP on
nrfConnect for now, since it is unused.
Fixes Issues #31779, #33307.