Skip to content
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

Fix PacketBuffer allocation size when using LwIP #8226

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

kpschoedel
Copy link
Contributor

Problem

LwIP pbuf_alloc() was called with a size including the struct pbuf
header, but expects the size to exclude that.

Change overview

Call pbuf_alloc() with the correct size.

This imports a fragment of openweave/openweave-core#608

Testing

Added PacketBuffer::kMaxSizeWithoutReserve to the cases checked
by PacketBufferTest::CheckNew().

#### Problem

LwIP `pbuf_alloc()` was called with a size including the `struct pbuf`
header, but expects the size to exclude that.

#### Change overview

Call `pbuf_alloc()` with the correct size.

This imports a fragment of openweave/openweave-core#608

#### Testing

Added `PacketBuffer::kMaxSizeWithoutReserve` to the cases checked
by `PacketBufferTest::CheckNew()`.
@github-actions
Copy link

github-actions bot commented Jul 9, 2021

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

File Section File VM
chip-shell.elf .flash.text -4 -4
chip-temperature-measurement-app.elf .flash.text -64 -64
chip-all-clusters-app.elf .flash.text -24 -24
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_loc,0,2
.debug_info,0,-2

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
[Unmapped],0,4
.debug_loc,0,2
.debug_info,0,-2
.flash.text,-4,-4

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
[Unmapped],0,64
.debug_info,0,-2
.debug_loc,0,-6
.flash.text,-64,-64

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

sections,vmsize,filesize
.xt.prop._ZN4chip6System5Mutex6UnlockEv,0,108
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,48
[Unmapped],0,24
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,12
.debug_loc,0,2
.debug_info,0,-2
.flash.text,-24,-24
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE14_InitChipStackEv,0,-40
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,-80
.xt.lit._ZN4chip6System5Mutex6UnlockEv,0,-128


@andy31415 andy31415 merged commit 5537680 into project-chip:master Jul 9, 2021
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this pull request Jul 9, 2021
#### Problem

1. After project-chip#8226, `PacketBuffer::kMaxSizeWithoutReserve` was wrong on LwIP builds.

2. It is possible for a `PacketBufferHandle::New()` to return a larger
   buffer than requested (e.g. when using a shared pool allocator), and
   in particular for it to return a block that is larger than *can* be
   requested. Attempting `CloneData()` on such a buffer fails with an
   error message logged by `PacketBufferHandle::New()`.

#### Change overview

1. Fix `PacketBuffer::kMaxSizeWithoutReserve`.

2. As long as the excess space is not actually occupied (which would be
   incorrect, since it exceeds the spec size limit), `CloneData()`
   copies correctly to a maximum-size buffer.

#### Testing

Added a unit test to verify that a buffer with excess unused space is
clonable, and a buffer with oversize space in use is not.
@kpschoedel kpschoedel deleted the pballoc branch July 9, 2021 16:47
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this pull request Jul 10, 2021
#### Problem

1. After project-chip#8226, `PacketBuffer::kMaxSizeWithoutReserve` was wrong on LwIP builds.

2. It is possible for a `PacketBufferHandle::New()` to return a larger
   buffer than requested (e.g. when using a shared pool allocator), and
   in particular for it to return a block that is larger than *can* be
   requested. Attempting `CloneData()` on such a buffer fails with an
   error message logged by `PacketBufferHandle::New()`.

#### Change overview

1. Fix `PacketBuffer::kMaxSizeWithoutReserve`.

2. As long as the excess space is not actually occupied (which would be
   incorrect, since it exceeds the spec size limit), `CloneData()`
   copies correctly to a maximum-size buffer.

#### Testing

Added a unit test to verify that a buffer with excess unused space is
clonable, and a buffer with oversize space in use is not.
andy31415 pushed a commit that referenced this pull request Jul 12, 2021
* Additional PacketBuffer size fixes.

#### Problem

1. After #8226, `PacketBuffer::kMaxSizeWithoutReserve` was wrong on LwIP builds.

2. It is possible for a `PacketBufferHandle::New()` to return a larger
   buffer than requested (e.g. when using a shared pool allocator), and
   in particular for it to return a block that is larger than *can* be
   requested. Attempting `CloneData()` on such a buffer fails with an
   error message logged by `PacketBufferHandle::New()`.

#### Change overview

1. Fix `PacketBuffer::kMaxSizeWithoutReserve`.

2. As long as the excess space is not actually occupied (which would be
   incorrect, since it exceeds the spec size limit), `CloneData()`
   copies correctly to a maximum-size buffer.

#### Testing

Added a unit test to verify that a buffer with excess unused space is
clonable, and a buffer with oversize space in use is not.

* dynamically allocate test buffer

* cast narrowing conversion
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
#### Problem

LwIP `pbuf_alloc()` was called with a size including the `struct pbuf`
header, but expects the size to exclude that.

#### Change overview

Call `pbuf_alloc()` with the correct size.

This imports a fragment of openweave/openweave-core#608

#### Testing

Added `PacketBuffer::kMaxSizeWithoutReserve` to the cases checked
by `PacketBufferTest::CheckNew()`.
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* Additional PacketBuffer size fixes.

#### Problem

1. After project-chip#8226, `PacketBuffer::kMaxSizeWithoutReserve` was wrong on LwIP builds.

2. It is possible for a `PacketBufferHandle::New()` to return a larger
   buffer than requested (e.g. when using a shared pool allocator), and
   in particular for it to return a block that is larger than *can* be
   requested. Attempting `CloneData()` on such a buffer fails with an
   error message logged by `PacketBufferHandle::New()`.

#### Change overview

1. Fix `PacketBuffer::kMaxSizeWithoutReserve`.

2. As long as the excess space is not actually occupied (which would be
   incorrect, since it exceeds the spec size limit), `CloneData()`
   copies correctly to a maximum-size buffer.

#### Testing

Added a unit test to verify that a buffer with excess unused space is
clonable, and a buffer with oversize space in use is not.

* dynamically allocate test buffer

* cast narrowing conversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants