Skip to content

gnrc_netif: don't allocate message queue on the stack #17905

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 13, 2022
Merged

gnrc_netif: don't allocate message queue on the stack #17905

merged 3 commits into from
Oct 13, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Apr 6, 2022 •
edited
Loading

Contribution description

The message queue of the netif was allocated on the netif stack, so we had to increase the netif stack size if we increase the message queue size.

CC110x was the only driver to get this right - add a new GNRC_NETIF_STACKSIZE_DEFAULT define so all drivers will get this right by default.

To properly fix this, add the message queue to gnrc_netif_t.

Testing procedure

This should not change the stack size except for one case: cc110x had CC110X_EXTRA_STACKSIZE set to (GNRC_NETIF_MSG_QUEUE_SIZE - 8) * sizeof(msg_t)) but the default message queue size is 16, adding (16 - 8) * 8 = 64 bytes to the stack.

Leaving this at 8 for all drivers would effectively bump their stack by 8 messages.
Maybe this was a mistake or maybe cc110x really needs the extra 64 bytes of stack.

@maribu might be able to give this a try and say if stack size is still sufficient.

Issues/PRs references

@benpicco benpicco requested a review from maribu April 6, 2022 12:19
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Apr 6, 2022
@benpicco benpicco requested a review from fabian18 April 6, 2022 12:19
@benpicco benpicco added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation and removed Area: network Area: Networking Area: sys Area: System labels Apr 6, 2022
@benpicco benpicco requested a review from jia200x April 7, 2022 12:54
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 24, 2022
@maribu
Copy link
Member

maribu commented Jun 3, 2022

After rebasing this on top of master I ran examples/gnrc_networking on our MIoT Nucleo-F767ZI boards:

	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
	 - | isr_stack            | -        - |   - |    512 (  256) (  256) | 0x20000000 | 0x200001c8
	 1 | main                 | running  Q |   7 |   1536 (  868) (  668) | 0x20000460 | 0x20000824 
	 2 | pktdump              | bl rx    _ |   6 |   1536 (  240) ( 1296) | 0x20004dd4 | 0x200052e4 
	 3 | 6lo                  | bl rx    _ |   3 |   1024 (  464) (  560) | 0x20005a24 | 0x20005c9c 
	 4 | ipv6                 | bl rx    _ |   4 |   1024 (  604) (  420) | 0x20000a64 | 0x20000cd4 
	 5 | udp                  | bl rx    _ |   5 |   1024 (  256) (  768) | 0x200061fc | 0x200064fc 
	 6 | stm32_eth            | bl anyfl _ |   2 |   1024 (  500) (  524) | 0x20002bd4 | 0x20002e94 
	 7 | at86rf215 [sub GHz]  | bl anyfl _ |   2 |   1024 (  576) (  448) | 0x20001288 | 0x200014bc 
	 8 | at86rf215 [2.4 GHz]  | bl anyfl _ |   2 |   1024 (  576) (  448) | 0x20001688 | 0x200018bc 
	 9 | cc110x               | bl anyfl _ |   2 |   1024 (  572) (  452) | 0x200020b4 | 0x200022ec 
	10 | nrf24l01p_ng         | bl anyfl _ |   2 |   1088 (  628) (  460) | 0x20002640 | 0x200028bc 
	11 | sx127x               | bl anyfl _ |   2 |   1024 (  468) (  556) | 0x200031b4 | 0x20003474 
	   | SUM                  |            |     |  12864 ( 6008) ( 6856)

The CC1101 driver seems to be roughly on par with the at86rf215 driver regarding stack consumption. So as it worked fine with the AT86RF215, it should work fine with the CC1101.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 3, 2022
@maribu maribu enabled auto-merge June 3, 2022 20:10
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 5, 2022
@miri64
Copy link
Member

miri64 commented Jun 5, 2022

Maybe, if the message queue size leads to problems, it is better to decouple it from the stack size, by making the message queue a static global?

@miri64 miri64 disabled auto-merge June 5, 2022 17:24
@benpicco
Copy link
Contributor Author

benpicco commented Jun 5, 2022 •
edited
Loading

You need one for every thread, so it can’t be static

@miri64
Copy link
Member

miri64 commented Jun 5, 2022

You need one for every thread, so it can’t be static

Ah, right! Sorry forgot about that ^^"

@miri64
Copy link
Member

miri64 commented Jun 5, 2022

Mhhh, but it could be part of the gnrc_netif struct :-D.

@benpicco benpicco mentioned this pull request Jun 6, 2022
Closed
@benpicco benpicco force-pushed the GNRC_NETIF_STACKSIZE_DEFAULT branch from d4246ce to f35e6bd Compare July 18, 2022 21:29
@benpicco
Copy link
Contributor Author

Mhhh, but it could be part of the gnrc_netif struct

I like the idea!

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 18, 2022
@miri64
Copy link
Member

miri64 commented Jul 19, 2022

Isn't that, what #18171 is for?

@benpicco benpicco force-pushed the GNRC_NETIF_STACKSIZE_DEFAULT branch from f35e6bd to b1ea963 Compare September 18, 2022 16:35
@maribu
Copy link
Member

maribu commented Sep 18, 2022

I thought #18171 would solve the issue this PR aims to solve?

#18171 is already ACKed, only the CI needs to be pleased yet.

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Oct 12, 2022
@benpicco
Copy link
Contributor Author

I like this one more as it also introduces a common GNRC_NETIF_STACKSIZE_DEFAULT

@maribu maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 13, 2022
@benpicco benpicco added this to the Release 2022.10 milestone Oct 13, 2022
@benpicco benpicco changed the title gnrc_netif: increase thread stack size when msg queue is increased gnrc_netif: don't allocate message queue on the stack Oct 13, 2022
@maribu maribu merged commit b539d7a into RIOT-OS:master Oct 13, 2022
@benpicco benpicco deleted the GNRC_NETIF_STACKSIZE_DEFAULT branch October 13, 2022 15:11
@benpicco benpicco mentioned this pull request Nov 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@fabian18 fabian18 Awaiting requested review from fabian18

@jia200x jia200x Awaiting requested review from jia200x

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @maribu @miri64