Reduce use of maximum-sized packet buffers #4434

kpschoedel · 2021-01-20T20:49:27Z

Problem

CHIP code contains many uses of PacketBuffer::New(), which allocates a
maximum-size buffer. This is often much larger than actually required,
which wastes heap.

Summary of Changes

Replaced PacketBuffer::New() and NewWithAvailableSize() with
PacketBufferHandle::New(), which requires an explicit packet size.
(The parameter order is opposite to the old NewWithAvailableSize()
because the reserved size is usually the default.) Made the available
parameter a size_t because for many callers it originates as such,
and New() already checks it; this allows removing CanCastTo tests
from callers. Also, documented the guarantee that New() never
returns a buffer with less space than requested, and removed post-New
size tests.
Added NewWithData() to encapsulate a common use of New(), and
reduce the risk of length errors between the steps.
Implemented RightSize() for the heap allocation case.
Added constants kMaxPacketBufferSizeWithoutReserve and
kMaxPacketBufferSize for use where the maximum size is required or
the size is unknown.
Added kMaxIPAddressStringLength for NetworkProvisioning::SendIPAddress.
Added NetworkProvisioning::EncodedStringSize() for SendNetworkCredentials().
Moved some size constants into BLE header files.

fixes #4351 - Reduce use of maximum-sized packet buffers

#### Problem CHIP code contains many uses of PacketBuffer::New(), which allocates a maximum-size buffer. This is often much larger than actually required, which wastes heap. #### Summary of Changes - Replaced `PacketBuffer::New()` and `NewWithAvailableSize()` with `PacketBufferHandle::New()`, which requires an explicit packet size. (The parameter order is opposite to the old `NewWithAvailableSize()` because the reserved size is usually the default.) Made the available parameter a `size_t` because for many callers it originates as such, and `New()` already checks it; this allows removing `CanCastTo` tests from callers. Also, documented the guarantee that `New()` never returns a buffer with less space than requested, and removed post-New size tests. - Added `NewWithData()` to encapsulate a common use of `New()`, and reduce the risk of length errors between the steps. - Implemented `RightSize()` for the heap allocation case. - Added constants `kMaxPacketBufferSizeWithoutReserve` and `kMaxPacketBufferSize` for use where the maximum size is required or the size is unknown. - Added `kMaxIPAddressStringLength` for `NetworkProvisioning::SendIPAddress`. - Added `NetworkProvisioning::EncodedStringSize()` for `SendNetworkCredentials()`. - Moved some size constants into BLE header files. fixes project-chip#4351 - Reduce use of maximum-sized packet buffers

kpschoedel · 2021-01-21T14:50:47Z

chip-all-clusters-app.elf .flash.text -64 -64

Dibs on these bytes to add a PacketBuffer+BufBound combo.

github-actions · 2021-01-22T22:42:49Z

Size increase report for "esp32-example-build" from ab81fbb

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	-72	-72

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,13249
.debug_line,0,960
.debug_abbrev,0,739
.debug_str,0,680
.debug_ranges,0,64
.shstrtab,0,1
.debug_aranges,0,-8
.symtab,0,-16
.debug_frame,0,-24
.strtab,0,-29
.flash.text,-72,-72
.debug_loc,0,-572

github-actions · 2021-01-22T22:43:12Z

Size increase report for "nrfconnect-example-build" from ab81fbb

File	Section	File	VM
chip-lock.elf	shell_root_cmds_sections	-4	-4
chip-lock.elf	text	-60	-60
chip-lighting.elf	shell_root_cmds_sections	-4	-4
chip-lighting.elf	text	-60	-60
chip-shell.elf	log_const_sections	-4	-4
chip-shell.elf	text	-12	-12

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,16257
.debug_abbrev,0,1743
.debug_str,0,680
.debug_line,0,444
.shstrtab,0,1
shell_root_cmds_sections,-4,-4
.debug_aranges,0,-8
.debug_ranges,0,-16
.debug_frame,0,-20
.strtab,0,-29
.symtab,0,-32
text,-60,-60
.debug_loc,0,-448

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,15957
.debug_abbrev,0,1766
.debug_str,0,680
.debug_line,0,453
.shstrtab,0,1
shell_root_cmds_sections,-4,-4
.debug_aranges,0,-8
.debug_ranges,0,-16
.debug_frame,0,-20
.strtab,0,-29
.symtab,0,-32
text,-60,-60
.debug_loc,0,-444

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,7853
.debug_abbrev,0,926
.debug_str,0,602
.shstrtab,0,2
log_const_sections,-4,-4
text,-12,-12
.debug_aranges,0,-16
.debug_line,0,-31
.debug_frame,0,-44
.symtab,0,-64
.strtab,0,-82
.debug_loc,0,-90
.debug_ranges,0,-104

rwalker-apple · 2021-01-26T17:13:23Z

@saurabhst @jelderton @BroderickCarlin @hawk248 ?

rwalker-apple · 2021-01-26T17:19:16Z

@kpschoedel conflicts?

tcarmelveilleux

LGTM overall, but please address comments, perhaps in a follow-up

tcarmelveilleux · 2021-01-26T17:18:41Z

src/inet/TCPEndPoint.cpp

@@ -2404,12 +2404,12 @@ void TCPEndPoint::ReceiveData()
    bool isNewBuf = true;

    if (mRcvQueue.IsNull())
-        rcvBuf = PacketBuffer::New(0);
+        rcvBuf = System::PacketBufferHandle::New(System::kMaxPacketBufferSizeWithoutReserve, 0);


This should actually be a "max TCP CHIP message size" cosntant.

I don't see an existing one; what's the right place for it?

tcarmelveilleux · 2021-01-26T17:20:05Z

src/lib/core/tests/TestCHIPTLV.cpp

@@ -1948,7 +1948,7 @@ void WriteDeleteReadTest(nlTestSuite * inSuite)
 */
 void CheckPacketBuffer(nlTestSuite * inSuite, void * inContext)
 {
-    System::PacketBufferHandle buf = System::PacketBuffer::New(0);
+    System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::kMaxPacketBufferSizeWithoutReserve, 0);


Again, we should have some more explicit size than this weird system default

tcarmelveilleux · 2021-01-26T17:21:05Z

src/messaging/ReliableMessageContext.cpp

@@ -344,7 +344,7 @@ CHIP_ERROR ReliableMessageContext::SendStandaloneAckMessage()
    CHIP_ERROR err = CHIP_NO_ERROR;

    // Allocate a buffer for the null message
-    System::PacketBufferHandle msgBuf = System::PacketBuffer::NewWithAvailableSize(0);
+    System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(0);


This doesn't make it clear that all overhead is swallowed in "reserved" default argument. Suggest making the default reserved argument explicit here.

tcarmelveilleux · 2021-01-26T17:22:31Z

src/messaging/tests/TestReliableMessageProtocol.cpp

    ctx.GetInetLayer().SystemLayer()->Init(nullptr);

-    chip::System::PacketBufferHandle buffer = chip::System::PacketBuffer::NewWithAvailableSize(payload_len);
+    chip::System::PacketBufferHandle buffer = chip::System::PacketBufferHandle::NewWithData(PAYLOAD, sizeof PAYLOAD, kMaxTagLen);


Weird that it's not sizeof(PAYLOAD)

sizeof is an operator with a special case for types; it's “sizeof expr” like “– expr”, or “sizeof (type)” like a cast. Of course “(expr)” is a valid expr, so I'll add the parens if it's distracting. (Surprised clang-format doesn't force it one way or the other.)

tcarmelveilleux · 2021-01-26T17:25:04Z

src/system/SystemPacketBuffer.cpp

+    // Reallocate only if enough space will be saved.
+    uint8_t * const start = reinterpret_cast<uint8_t *>(mBuffer) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE;
+    uint8_t * const payload = reinterpret_cast<uint8_t *>(mBuffer->payload);
+    const uint16_t usedSize = static_cast<uint16_t>(payload - start + mBuffer->len);


This should use size_t, and only use downscaling to uint16_t if needed.=

tcarmelveilleux · 2021-01-26T17:25:41Z

src/system/SystemPacketBuffer.cpp

-    memcpy(other->Start(), mBuffer->Start(), mBuffer->DataLength());
-    other->SetDataLength(mBuffer->DataLength());
+    const uint16_t blockSize = usedSize + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE;
+    PacketBuffer * newBuffer = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(blockSize));


Could this use a Realloc-like API (did not check if we have it) followed by a memmove below?

tcarmelveilleux · 2021-01-26T17:28:36Z

src/transport/NetworkProvisioning.cpp

@@ -124,6 +124,11 @@ CHIP_ERROR NetworkProvisioning::HandleNetworkProvisioningMessage(uint8_t msgType
    return err;
 }

+uint16_t NetworkProvisioning::EncodedStringSize(const char * str)
+{
+    return static_cast<uint16_t>(strlen(str) + sizeof(uint16_t));


Suggested change

return static_cast<uint16_t>(strlen(str) + sizeof(uint16_t));

return static_cast<uint16_t>(sizeof(uint16_t) + strlen(str));

Also suggest using size_t here, not uint16_t, or at least internally use size_t and ensure it fits within 16 bits

tcarmelveilleux · 2021-01-26T17:29:39Z

src/transport/NetworkProvisioning.cpp

+    System::PacketBufferHandle buffer = System::PacketBufferHandle::New(Inet::kMaxIPAddressStringLength);
+    VerifyOrExit(!buffer.IsNull(), err = CHIP_ERROR_NO_MEMORY);
+    addrStr = addr.ToString(Uint8::to_char(buffer->Start()), buffer->AvailableDataLength());
+    buffer->SetDataLength(static_cast<uint16_t>(strlen(addrStr) + 1));


Why the +1? Should this be +2 ?

It matches what the code did, but maybe that's wrong. Is it supposed to use EncodeString?

tcarmelveilleux · 2021-01-26T17:31:52Z

src/system/SystemPacketBuffer.cpp

+    newBuffer->len = mBuffer->len;
+    newBuffer->ref = 1;
+    newBuffer->alloc_size = static_cast<uint16_t>(usedSize);
+    memcpy(reinterpret_cast<uint8_t *>(newBuffer) + CHIP_SYSTEM_PACKETBUFFER_HEADER_SIZE, start, usedSize);


This doesn't follow the chain. Rightsizing is to get a single linear buffer of correct chain from a single PacketBuffer of size > optiomal OR from a chain. This would need to take in account the chain, otherwise this may copy past the end of the buffer. I am surprised to see ->len being used directly, rather than looking at the entire chain (e.g. equivalent of TotalLength)

Yes. I thought I had written that, but evidently didn't (or misplaced it on another branch). Will move to draft until I add it.

Memory has been refreshed. The LwIP implementation also does not support chains, so I added the comment to call CompactHead() first for that case. If RightSize() itself needs to handle chains, I'd rather land that separately.

woody-apple · 2021-01-27T05:04:47Z

@kpschoedel there are a ton of merge conflicts here, can you resolve and re-request review? Thanks!

woody-apple · 2021-01-28T19:01:57Z

@tcarmelveilleux - does this look good to you now?
@kpschoedel - should this still be draft, or ready for merge?

@saurabhst @jelderton can you folks take a peek too?

kpschoedel · 2021-01-28T19:05:49Z

@tcarmelveilleux - does this look good to you now?
@kpschoedel - should this still be draft, or ready for merge?

Still draft; I still want to merge over the correct RightSize() from my local heap allocation branch. That should happen soon.

kpschoedel · 2021-01-28T19:17:00Z

Actually, changed my mind on that.

woody-apple · 2021-01-28T20:53:09Z

Appears all @tcarmelveilleux 's comments are addressed. Approving/merging.

* Reduce use of maximum-sized packet buffers CHIP code contains many uses of PacketBuffer::New(), which allocates a maximum-size buffer. This is often much larger than actually required, which wastes heap. - Replaced `PacketBuffer::New()` and `NewWithAvailableSize()` with `PacketBufferHandle::New()`, which requires an explicit packet size. (The parameter order is opposite to the old `NewWithAvailableSize()` because the reserved size is usually the default.) Made the available parameter a `size_t` because for many callers it originates as such, and `New()` already checks it; this allows removing `CanCastTo` tests from callers. Also, documented the guarantee that `New()` never returns a buffer with less space than requested, and removed post-New size tests. - Added `NewWithData()` to encapsulate a common use of `New()`, and reduce the risk of length errors between the steps. - Implemented `RightSize()` for the heap allocation case. - Added constants `kMaxPacketBufferSizeWithoutReserve` and `kMaxPacketBufferSize` for use where the maximum size is required or the size is unknown. - Added `kMaxIPAddressStringLength` for `NetworkProvisioning::SendIPAddress`. - Added `NetworkProvisioning::EncodedStringSize()` for `SendNetworkCredentials()`. - Moved some size constants into BLE header files. fixes project-chip#4351 - Reduce use of maximum-sized packet buffers * Fix merge of remote-tracking branch 'origin/master' into x4351-new * review * restyled

* Reduce use of maximum-sized packet buffers #### Problem CHIP code contains many uses of PacketBuffer::New(), which allocates a maximum-size buffer. This is often much larger than actually required, which wastes heap. #### Summary of Changes - Replaced `PacketBuffer::New()` and `NewWithAvailableSize()` with `PacketBufferHandle::New()`, which requires an explicit packet size. (The parameter order is opposite to the old `NewWithAvailableSize()` because the reserved size is usually the default.) Made the available parameter a `size_t` because for many callers it originates as such, and `New()` already checks it; this allows removing `CanCastTo` tests from callers. Also, documented the guarantee that `New()` never returns a buffer with less space than requested, and removed post-New size tests. - Added `NewWithData()` to encapsulate a common use of `New()`, and reduce the risk of length errors between the steps. - Implemented `RightSize()` for the heap allocation case. - Added constants `kMaxPacketBufferSizeWithoutReserve` and `kMaxPacketBufferSize` for use where the maximum size is required or the size is unknown. - Added `kMaxIPAddressStringLength` for `NetworkProvisioning::SendIPAddress`. - Added `NetworkProvisioning::EncodedStringSize()` for `SendNetworkCredentials()`. - Moved some size constants into BLE header files. fixes project-chip#4351 - Reduce use of maximum-sized packet buffers * Fix merge of remote-tracking branch 'origin/master' into x4351-new * review * restyled

As of a868187 ("Reduce use of maximum-sized packet buffers (project-chip#4434)"), network provisioning does not allocate enough space to encrypt the WiFi credentials payload, resulting in the following error: CHIP:NP: Failed in sending Network Creds. error Error 4047 (0x00000FCF) Pending API improvements to make these errors less likely, manually add the needed extra space at allocation time. Fixes project-chip#4823

As of a868187 ("Reduce use of maximum-sized packet buffers (#4434)"), network provisioning does not allocate enough space to encrypt the WiFi credentials payload, resulting in the following error: CHIP:NP: Failed in sending Network Creds. error Error 4047 (0x00000FCF) Pending API improvements to make these errors less likely, manually add the needed extra space at allocation time. Fixes #4823

boring-cyborg bot added app ble controller darwin efr32 esp32 examples inet lib platform labels Jan 20, 2021

kpschoedel marked this pull request as ready for review January 20, 2021 21:06

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple, saurabhst and shana-apple January 20, 2021 21:06

andy31415 approved these changes Jan 21, 2021

View reviewed changes

kpschoedel added 2 commits January 22, 2021 16:40

Merge remote-tracking branch 'origin/master' into x4351-new

33a0138

Fix merge of remote-tracking branch 'origin/master' into x4351-new

62639dc

rwalker-apple approved these changes Jan 26, 2021

View reviewed changes

saurabhst approved these changes Jan 26, 2021

View reviewed changes

tcarmelveilleux approved these changes Jan 26, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into x4351-new

3bf67f0

woody-apple requested review from tcarmelveilleux and saurabhst January 27, 2021 05:04

pullapprove bot requested a review from woody-apple January 27, 2021 05:28

Merge remote-tracking branch 'origin/master' into x4351-new

f158fe6

kpschoedel marked this pull request as draft January 27, 2021 21:40

review

6e1bc7c

woody-apple approved these changes Jan 28, 2021

View reviewed changes

saurabhst approved these changes Jan 28, 2021

View reviewed changes

restyled-io bot mentioned this pull request Jan 28, 2021

Restyle Reduce use of maximum-sized packet buffers #4553

Closed

kpschoedel marked this pull request as ready for review January 28, 2021 19:21

restyled

044cbb2

woody-apple merged commit a868187 into project-chip:master Jan 28, 2021

kpschoedel deleted the x4351-new branch January 29, 2021 19:42

bzbarsky-apple mentioned this pull request Feb 3, 2021

PacketBufferHandle::NewWithData and PacketBufferHandle::New have integer overflow issues #4637

Closed

mspang mentioned this pull request Feb 15, 2021

Fix missing footer space during WiFi provisioning #4868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce use of maximum-sized packet buffers #4434

Reduce use of maximum-sized packet buffers #4434

kpschoedel commented Jan 20, 2021

kpschoedel commented Jan 21, 2021

github-actions bot commented Jan 22, 2021

github-actions bot commented Jan 22, 2021

rwalker-apple commented Jan 26, 2021

rwalker-apple commented Jan 26, 2021

tcarmelveilleux left a comment

tcarmelveilleux Jan 26, 2021

kpschoedel Jan 27, 2021

tcarmelveilleux Jan 26, 2021

tcarmelveilleux Jan 26, 2021

kpschoedel Jan 27, 2021

tcarmelveilleux Jan 26, 2021

kpschoedel Jan 27, 2021

tcarmelveilleux Jan 26, 2021

kpschoedel Jan 27, 2021

tcarmelveilleux Jan 26, 2021

tcarmelveilleux Jan 26, 2021

tcarmelveilleux Jan 26, 2021

kpschoedel Jan 28, 2021

tcarmelveilleux Jan 26, 2021

kpschoedel Jan 27, 2021

kpschoedel Jan 28, 2021

woody-apple commented Jan 27, 2021

woody-apple commented Jan 28, 2021

kpschoedel commented Jan 28, 2021

kpschoedel commented Jan 28, 2021

woody-apple commented Jan 28, 2021

	return static_cast<uint16_t>(strlen(str) + sizeof(uint16_t));
	return static_cast<uint16_t>(sizeof(uint16_t) + strlen(str));

Reduce use of maximum-sized packet buffers #4434

Reduce use of maximum-sized packet buffers #4434

Conversation

kpschoedel commented Jan 20, 2021

Problem

Summary of Changes

kpschoedel commented Jan 21, 2021

github-actions bot commented Jan 22, 2021

github-actions bot commented Jan 22, 2021

rwalker-apple commented Jan 26, 2021

rwalker-apple commented Jan 26, 2021

tcarmelveilleux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woody-apple commented Jan 27, 2021

woody-apple commented Jan 28, 2021

kpschoedel commented Jan 28, 2021

kpschoedel commented Jan 28, 2021

woody-apple commented Jan 28, 2021