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

PacketBuffer: Make discontiguous LwIP pbuf adoptable #25679

Merged
merged 2 commits into from
Mar 24, 2023

Conversation

wqx6
Copy link
Contributor

@wqx6 wqx6 commented Mar 14, 2023

Changes

Make the discontiguous LwIP pbuf adoptable to reduce the PacketBuffer: pool EMPTY. errors.

Tests

Tested on ESP32, both the unicast and multicast message can be received and handled well.

@github-actions
Copy link

PR #25679: Size comparison from c1810b6 to 6be1d70

Increases (1 build for cc32xx)
platform target config section c1810b6 6be1d70 change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_abbrev 930293 930309 16 0.0
.debug_frame 300248 300252 4 0.0
Decreases (1 build for cc32xx)
platform target config section c1810b6 6be1d70 change % change
cc32xx lock CC3235SF_LAUNCHXL (read only) 645489 645361 -128 -0.0
.debug_info 20245969 20239523 -6446 -0.0
.debug_line 2660979 2660526 -453 -0.0
.debug_loc 2804911 2803881 -1030 -0.0
.debug_str 3026947 3020209 -6738 -0.2
.rodata 106009 105969 -40 -0.0
.text 537360 537272 -88 -0.0
Full report (1 build for cc32xx)
platform target config section c1810b6 6be1d70 change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 645489 645361 -128 -0.0
(read/write) 203848 203848 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197248 197248 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930293 930309 16 0.0
.debug_aranges 87384 87384 0 0.0
.debug_frame 300248 300252 4 0.0
.debug_info 20245969 20239523 -6446 -0.0
.debug_line 2660979 2660526 -453 -0.0
.debug_loc 2804911 2803881 -1030 -0.0
.debug_ranges 283168 283168 0 0.0
.debug_str 3026947 3020209 -6738 -0.2
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 106009 105969 -40 -0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 380271 380271 0 0.0
.symtab 257312 257312 0 0.0
.text 537360 537272 -88 -0.0

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes #21816, right?

It looks to me like the CloneData bits described in #20923 (comment) are not an issue because CloneData only uses public APIs that this PR fixes, right?

src/system/SystemPacketBuffer.cpp Outdated Show resolved Hide resolved
src/system/SystemPacketBuffer.cpp Outdated Show resolved Hide resolved
src/system/SystemPacketBuffer.cpp Outdated Show resolved Hide resolved
@wqx6 wqx6 force-pushed the packetbuffer_discontiguous branch from 6be1d70 to c0f04fa Compare March 15, 2023 01:53
@wqx6 wqx6 force-pushed the packetbuffer_discontiguous branch from c0f04fa to 219bb4f Compare March 15, 2023 01:56
@github-actions
Copy link

PR #25679: Size comparison from 12bcb2c to 219bb4f

Increases (1 build for cc32xx)
platform target config section 12bcb2c 219bb4f change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_abbrev 930293 930309 16 0.0
.debug_frame 300248 300252 4 0.0
Decreases (1 build for cc32xx)
platform target config section 12bcb2c 219bb4f change % change
cc32xx lock CC3235SF_LAUNCHXL (read only) 645489 645361 -128 -0.0
.debug_info 20245969 20239594 -6375 -0.0
.debug_line 2660979 2660564 -415 -0.0
.debug_loc 2804911 2803923 -988 -0.0
.debug_str 3026947 3020209 -6738 -0.2
.rodata 106009 105969 -40 -0.0
.text 537360 537272 -88 -0.0
Full report (1 build for cc32xx)
platform target config section 12bcb2c 219bb4f change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 645489 645361 -128 -0.0
(read/write) 203848 203848 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197248 197248 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930293 930309 16 0.0
.debug_aranges 87384 87384 0 0.0
.debug_frame 300248 300252 4 0.0
.debug_info 20245969 20239594 -6375 -0.0
.debug_line 2660979 2660564 -415 -0.0
.debug_loc 2804911 2803923 -988 -0.0
.debug_ranges 283168 283168 0 0.0
.debug_str 3026947 3020209 -6738 -0.2
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 106009 105969 -40 -0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 380271 380271 0 0.0
.symtab 257312 257312 0 0.0
.text 537360 537272 -88 -0.0

@wqx6
Copy link
Contributor Author

wqx6 commented Mar 15, 2023

This fixes #21816, right?

It looks to me like the CloneData bits described in #20923 (comment) are not an issue because CloneData only uses public APIs that this PR fixes, right?

we might run out of PBUF pool if we clone the udp message. After we increase the event queue, the error PacketBuffer: pool EMPTY happens when the ScheduleLambda is block and the udp endpoint receives a lot of packets. Described in #23180 comment.

@github-actions
Copy link

github-actions bot commented Mar 15, 2023

PR #25679: Size comparison from 12bcb2c to 23c457b

Increases (15 builds for bl702, cc13x2_26x2, cc32xx, linux, nrfconnect, qpg)
platform target config section 12bcb2c 23c457b change % change
bl702 lighting-app bl702 .debug_abbrev 1556799 1556834 35 0.0
.debug_frame 493292 493304 12 0.0
bl702+rpc .debug_abbrev 1708914 1708949 35 0.0
.debug_frame 521236 521248 12 0.0
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read/write) 169200 169288 88 0.1
lock-ftd LP_CC2652R7 (read/write) 169504 169576 72 0.0
lock-mtd LP_CC2652R7 (read/write) 178700 178764 64 0.0
pump-app LP_CC2652R7 (read/write) 157420 157500 80 0.1
pump-controller-app LP_CC2652R7 (read/write) 172620 172700 80 0.0
shell LP_CC2652R7 (read/write) 179832 179936 104 0.1
cc32xx lock CC3235SF_LAUNCHXL .debug_abbrev 930293 930309 16 0.0
.debug_frame 300248 300252 4 0.0
linux thermostat-no-ble arm64 (read only) 2523340 2523356 16 0.0
.text 2110032 2110048 16 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 text 804636 804644 8 0.0
nrf7002dk_nrf5340_cpuapp (read/write) 1433600 1433616 16 0.0
text 775556 775564 8 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 (read/write) 1118796 1118812 16 0.0
text 774004 774016 12 0.0
qpg lighting-app qpg6105+debug (read/write) 1153444 1153452 8 0.0
.text 600540 600548 8 0.0
lock-app qpg6105+debug (read/write) 1120300 1120308 8 0.0
.text 567400 567408 8 0.0
Decreases (12 builds for bl602, bl702, cc13x2_26x2, cc32xx)
platform target config section 12bcb2c 23c457b change % change
bl602 lighting-app bl602 (read/write) 1356586 1356482 -104 -0.0
.text 1028960 1028892 -68 -0.0
bl602+rpc (read/write) 1402010 1401906 -104 -0.0
.text 1059894 1059826 -68 -0.0
bl702 lighting-app bl702 (read/write) 1188811 1188571 -240 -0.0
.debug_info 40833271 40827722 -5549 -0.0
.debug_line 5294996 5294610 -386 -0.0
.debug_loc 3426764 3426411 -353 -0.0
.debug_str 3588025 3581287 -6738 -0.2
.rodata 108032 108000 -32 -0.0
.strtab 577184 577166 -18 -0.0
.symtab 174224 174208 -16 -0.0
.text 959450 959248 -202 -0.0
bl702+rpc (read/write) 1279479 1279239 -240 -0.0
.debug_info 45459775 45454226 -5549 -0.0
.debug_line 5700339 5699953 -386 -0.0
.debug_loc 3624755 3624402 -353 -0.0
.debug_str 3992214 3985476 -6738 -0.2
.rodata 122240 122208 -32 -0.0
.strtab 638509 638491 -18 -0.0
.symtab 192720 192704 -16 -0.0
.text 1034792 1034590 -202 -0.0
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 682055 681967 -88 -0.0
.rodata 88551 88519 -32 -0.0
.text 593188 593132 -56 -0.0
all-clusters-minimal-app LP_CC2652R7 (read only) 645367 645271 -96 -0.0
.rodata 78423 78391 -32 -0.0
.text 566624 566560 -64 -0.0
lock-ftd LP_CC2652R7 (read only) 679215 679143 -72 -0.0
.rodata 76791 76759 -32 -0.0
.text 601944 601904 -40 -0.0
lock-mtd LP_CC2652R7 (read only) 665179 665115 -64 -0.0
.rodata 103595 103563 -32 -0.0
.text 561104 561072 -32 -0.0
pump-app LP_CC2652R7 (read only) 692043 691963 -80 -0.0
.rodata 91019 90979 -40 -0.0
.text 600544 600504 -40 -0.0
pump-controller-app LP_CC2652R7 (read only) 676979 676899 -80 -0.0
.rodata 86891 86851 -40 -0.0
.text 589608 589568 -40 -0.0
shell LP_CC2652R7 (read only) 673494 673390 -104 -0.0
.rodata 85454 85414 -40 -0.0
.text 587728 587664 -64 -0.0
cc32xx lock CC3235SF_LAUNCHXL (read only) 645489 645361 -128 -0.0
.debug_info 20245969 20239594 -6375 -0.0
.debug_line 2660979 2660564 -415 -0.0
.debug_loc 2804911 2803923 -988 -0.0
.debug_str 3026947 3020209 -6738 -0.2
.rodata 106009 105969 -40 -0.0
.text 537360 537272 -88 -0.0
Full report (20 builds for bl602, bl702, cc13x2_26x2, cc32xx, linux, mbed, nrfconnect, qpg)
platform target config section 12bcb2c 23c457b change % change
bl602 lighting-app bl602 (read/write) 1356586 1356482 -104 -0.0
.bss 94842 94842 0 0.0
.data 9744 9744 0 0.0
.text 1028960 1028892 -68 -0.0
bl602+rpc (read/write) 1402010 1401906 -104 -0.0
.bss 102890 102890 0 0.0
.data 10136 10136 0 0.0
.text 1059894 1059826 -68 -0.0
bl702 lighting-app bl702 (read only) 3358 3358 0 0.0
(read/write) 1188811 1188571 -240 -0.0
.bleromro 6342 6342 0 0.0
.bleromrw 124 124 0 0.0
.boot2 292 292 0 0.0
.bss 70113 70113 0 0.0
.bss_psram 26048 26048 0 0.0
.comment 48 48 0 0.0
.data 4072 4072 0 0.0
.debug_abbrev 1556799 1556834 35 0.0
.debug_aranges 134584 134584 0 0.0
.debug_frame 493292 493304 12 0.0
.debug_info 40833271 40827722 -5549 -0.0
.debug_line 5294996 5294610 -386 -0.0
.debug_loc 3426764 3426411 -353 -0.0
.debug_ranges 373512 373512 0 0.0
.debug_str 3588025 3581287 -6738 -0.2
.hbn 536 536 0 0.0
.hbn_noinit 260 260 0 0.0
.init 342 342 0 0.0
.init_array 144 144 0 0.0
.psram 0 0 0 0.0
.riscv.attributes 47 47 0 0.0
.rodata 108032 108000 -32 -0.0
.rsvd 2960 2960 0 0.0
.sha_ocram 72 72 0 0.0
.shstrtab 304 304 0 0.0
.stack 2048 2048 0 0.0
.strtab 577184 577166 -18 -0.0
.symtab 174224 174208 -16 -0.0
.tcm_data 36 36 0 0.0
.tcmcode 3358 3358 0 0.0
.text 0 0 0 0.0
959450 959248 -202 -0.0
bl702+rpc (read only) 3358 3358 0 0.0
(read/write) 1279479 1279239 -240 -0.0
.bleromro 6342 6342 0 0.0
.bleromrw 124 124 0 0.0
.boot2 292 292 0 0.0
.bss 78161 78161 0 0.0
.bss_psram 26320 26320 0 0.0
.comment 48 48 0 0.0
.data 4616 4616 0 0.0
.debug_abbrev 1708914 1708949 35 0.0
.debug_aranges 142912 142912 0 0.0
.debug_frame 521236 521248 12 0.0
.debug_info 45459775 45454226 -5549 -0.0
.debug_line 5700339 5699953 -386 -0.0
.debug_loc 3624755 3624402 -353 -0.0
.debug_ranges 397512 397512 0 0.0
.debug_str 3992214 3985476 -6738 -0.2
.hbn 536 536 0 0.0
.hbn_noinit 260 260 0 0.0
.init 342 342 0 0.0
.init_array 160 160 0 0.0
.psram 0 0 0 0.0
.riscv.attributes 47 47 0 0.0
.rodata 122240 122208 -32 -0.0
.rsvd 2960 2960 0 0.0
.sha_ocram 72 72 0 0.0
.shstrtab 304 304 0 0.0
.stack 2048 2048 0 0.0
.strtab 638509 638491 -18 -0.0
.symtab 192720 192704 -16 -0.0
.tcm_data 36 36 0 0.0
.tcmcode 3358 3358 0 0.0
.text 0 0 0 0.0
1034792 1034590 -202 -0.0
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 682055 681967 -88 -0.0
(read/write) 169200 169288 88 0.1
.bss 80948 80948 0 0.0
.data 3352 3352 0 0.0
.rodata 88551 88519 -32 -0.0
.text 593188 593132 -56 -0.0
all-clusters-minimal-app LP_CC2652R7 (read only) 645367 645271 -96 -0.0
(read/write) 157616 157616 0 0.0
.bss 80148 80148 0 0.0
.data 3352 3352 0 0.0
.rodata 78423 78391 -32 -0.0
.text 566624 566560 -64 -0.0
lock-ftd LP_CC2652R7 (read only) 679215 679143 -72 -0.0
(read/write) 169504 169576 72 0.0
.bss 78500 78500 0 0.0
.data 3316 3316 0 0.0
.rodata 76791 76759 -32 -0.0
.text 601944 601904 -40 -0.0
lock-mtd LP_CC2652R7 (read only) 665179 665115 -64 -0.0
(read/write) 178700 178764 64 0.0
.bss 73660 73660 0 0.0
.data 3316 3316 0 0.0
.rodata 103595 103563 -32 -0.0
.text 561104 561072 -32 -0.0
pump-app LP_CC2652R7 (read only) 692043 691963 -80 -0.0
(read/write) 157420 157500 80 0.1
.bss 78476 78476 0 0.0
.data 3280 3280 0 0.0
.rodata 91019 90979 -40 -0.0
.text 600544 600504 -40 -0.0
pump-controller-app LP_CC2652R7 (read only) 676979 676899 -80 -0.0
(read/write) 172620 172700 80 0.0
.bss 78612 78612 0 0.0
.data 3304 3304 0 0.0
.rodata 86891 86851 -40 -0.0
.text 589608 589568 -40 -0.0
shell LP_CC2652R7 (read only) 673494 673390 -104 -0.0
(read/write) 179832 179936 104 0.1
.bss 83020 83020 0 0.0
.data 3348 3348 0 0.0
.rodata 85454 85414 -40 -0.0
.text 587728 587664 -64 -0.0
cc32xx lock CC3235SF_LAUNCHXL (read only) 645489 645361 -128 -0.0
(read/write) 203848 203848 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197248 197248 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930293 930309 16 0.0
.debug_aranges 87384 87384 0 0.0
.debug_frame 300248 300252 4 0.0
.debug_info 20245969 20239594 -6375 -0.0
.debug_line 2660979 2660564 -415 -0.0
.debug_loc 2804911 2803923 -988 -0.0
.debug_ranges 283168 283168 0 0.0
.debug_str 3026947 3020209 -6738 -0.2
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 106009 105969 -40 -0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 380271 380271 0 0.0
.symtab 257312 257312 0 0.0
.text 0 0 0 0.0
537360 537272 -88 -0.0
linux chip-tool-ipv6only arm64 (read only) 12174332 12174332 0 0.0
(read/write) 733432 733432 0 0.0
.bss 34296 34296 0 0.0
.data 3008 3008 0 0.0
.data.rel.ro 675528 675528 0 0.0
.dynamic 560 560 0 0.0
.got 15392 15392 0 0.0
.init 24 24 0 0.0
.init_array 216 216 0 0.0
.rodata 589420 589420 0 0.0
.text 9844324 9844324 0 0.0
thermostat-no-ble arm64 (read only) 2523340 2523356 16 0.0
(read/write) 145240 145240 0 0.0
.bss 56344 56344 0 0.0
.data 1784 1784 0 0.0
.data.rel.ro 77696 77696 0 0.0
.dynamic 560 560 0 0.0
.got 5368 5368 0 0.0
.init 24 24 0 0.0
.init_array 432 432 0 0.0
.rodata 150920 150920 0 0.0
.text 2110032 2110048 16 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2468312 2468312 0 0.0
.bss 215964 215964 0 0.0
.data 5880 5880 0 0.0
.text 1430956 1430956 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read only) 4 4 0 0.0
(read/write) 1173580 1173580 0 0.0
bss 155557 155557 0 0.0
rodata 132620 132620 0 0.0
text 804636 804644 8 0.0
nrf7002dk_nrf5340_cpuapp (read only) 4 4 0 0.0
(read/write) 1433600 1433616 16 0.0
bss 135297 135297 0 0.0
rodata 228672 228672 0 0.0
text 775556 775564 8 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 (read only) 4 4 0 0.0
(read/write) 1118796 1118812 16 0.0
bss 154713 154713 0 0.0
rodata 109436 109436 0 0.0
text 774004 774016 12 0.0
qpg lighting-app qpg6105+debug (read/write) 1153444 1153452 8 0.0
.bss 100076 100076 0 0.0
.data 852 852 0 0.0
.text 600540 600548 8 0.0
lock-app qpg6105+debug (read/write) 1120300 1120308 8 0.0
.bss 96468 96468 0 0.0
.data 864 864 0 0.0
.text 567400 567408 8 0.0

@bzbarsky-apple
Copy link
Contributor

we might run out of PBUF pool if we clone the udp message.

Yes, I understand that. My questions are:

  1. Does this fix Try to reuse the lwIP pbuf in UDPEndPointImplLwIP.cpp #21816?
  2. With these changes, is calling CloneData on a non-contiguous buffer that's been adopted (which some of our internals will end up doing) going to do the right thing?

@bzbarsky-apple
Copy link
Contributor

And perhaps most concretely: was receiving group messages tested with this change?

@wqx6
Copy link
Contributor Author

wqx6 commented Mar 16, 2023

Does this fix #21816?

Yes

With these changes, is calling CloneData on a non-contiguous buffer that's been adopted (which some of our internals will end up doing) going to do the right thing?

Yes, we will create a pbuf with the reserve size 0 in CloneData and copy the payload. Same as https://github.com/project-chip/connectedhomeip/pull/25679/files#diff-d518c09d6ad62381f954eb9520bb175375404f40576a60fa6b25f37bc1c288c0L423.

was receiving group messages tested with this change?

Yes, I have tested the group messages receiving on ESP32.

@bzbarsky-apple bzbarsky-apple linked an issue Mar 17, 2023 that may be closed by this pull request
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That all matches what I was seeing too!

@andy31415 andy31415 merged commit 77e39a6 into project-chip:master Mar 24, 2023
hawkhan pushed a commit to Ayla-Professional-Service/csa_matter that referenced this pull request Dec 20, 2023
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.

Try to reuse the lwIP pbuf in UDPEndPointImplLwIP.cpp
3 participants