-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes for large Packetbuffer allocation to support TCP payloads #33308
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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)
|
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)
|
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)
|
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)
|
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)
|
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? |
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. |
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)
|
bfa802e
to
ffbd7d3
Compare
PR #33308: Size comparison from e0bdb09 to ffbd7d3 Increases (19 builds for esp32, linux, nrfconnect, nxp, stm32)
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)
|
ffbd7d3
to
a1816de
Compare
PR #33308: Size comparison from d8e7e43 to a1816de Full report (6 builds for mbed, qpg, stm32, tizen)
|
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 project-chip#31779.
a1816de
to
5261df4
Compare
PR #33308: Size comparison from d8e7e43 to 5261df4 Full report (8 builds for cc32xx, mbed, qpg, stm32, tizen)
|
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
PR #33308: Size comparison from d8e7e43 to 9e379b5 Full report (6 builds for cc32xx, mbed, stm32, tizen)
|
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
PR #33308: Size comparison from d8e7e43 to b0063fa Full report (96 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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 #31779.