Changes for large Packetbuffer allocation to support TCP payloads #33308
+126
−68
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
kpschoedel,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda,
mhazley,
mkardous-silabs,
mspang,
nivi-apple,
p0fi,
plauric,
rcasallas-silabs,
robszewczyk,
saurabhst,
selissia,
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)
|
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.
Fixes Issues #31779, #33307.