Skip to content

gnrc: make message queues static#19998

Merged
maribu merged 6 commits intoRIOT-OS:masterfrom
fabian18:gnrc_static_messge_queues
Dec 4, 2023
Merged

gnrc: make message queues static#19998
maribu merged 6 commits intoRIOT-OS:masterfrom
fabian18:gnrc_static_messge_queues

Conversation

@fabian18
Copy link
Copy Markdown
Contributor

@fabian18 fabian18 commented Oct 19, 2023

Contribution description

I saw messages sent to the IPv6 thread being dropped.
I think it could happen that an address does not become valid for that reason.
So I increased CONFIG_GNRC_IPV6_MSG_QUEUE_SIZE_EXP and a stack overflow happened.
This is not supposed to happen for a CONFIG_ variable.

Testing procedure

  • internal application
2023-10-19 17:19:17,118 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:17,148 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:17,149 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:17,422 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:17,438 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:17,693 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:17,724 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:17,950 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:18,029 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:18,318 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:18,332 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:18,589 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:18,590 # queue_msg(): Cannot send message 0x2000a7b0 to message queue of thread 4 is full (or there is none)
2023-10-19 17:19:18,798 # queue_msg(): Cannot send message 0x20008888 to message queue of thread 4 is full (or there is none)

2023-10-19 17:32:33,253 # ps
2023-10-19 17:32:33,270 # 	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
2023-10-19 17:32:33,272 # 	  - | isr_stack            | -        - |   - |   2048 (  492) ( 1556) | 0x20000000 | 0x200007c8
2023-10-19 17:32:33,287 # 	  2 | pktdump              | bl rx    _ |   6 |   5120 (  240) ( 4880) | 0x2001b0ec | 0x2001c3fc 
2023-10-19 17:32:33,301 # 	  3 | 6lo                  | bl rx    _ |   3 |   1152 (  416) (  736) | 0x2001c74c | 0x2001ca44 
2023-10-19 17:32:33,303 # 	  4 | ipv6                 | bl rx    _ |   4 |   1152 (  844) (  308) | 0x2000854c | 0x2000883c 
2023-10-19 17:32:33,318 # 	  5 | udp                  | bl rx    _ |   5 |    576 (  312) (  264) | 0x2001cbcc | 0x2001cd0c

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: sys Area: System labels Oct 19, 2023
@fabian18 fabian18 changed the title gnrc: make message queueus static gnrc: make message queues static Oct 19, 2023
@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 Oct 19, 2023
@riot-ci
Copy link
Copy Markdown

riot-ci commented Oct 19, 2023 •
edited
Loading

Murdock results

✔️ PASSED

0cc2bda sys/include/net/gnrc: reduce stack by previously allocated message queue

Success Failures Total Runtime
8082 0 8082 17m:50s

Artifacts

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 20, 2023 •
edited
Loading

Mhh... to what did you increase the queue size that it overflowed the stack? Does it make sense to increase the stack size as well then?

The problem with adding the queue to static memory for all modules is that this effectively increases the RAM usage of GNRC. Does it make sense to do this only for IPv6 for now?

miri64
miri64 reviewed Oct 20, 2023
View reviewed changes
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 20, 2023

You should then reduce the stack size of all GNRC threads by the default queue size to avoid increasing the memory requirements.

@fabian18
Copy link
Copy Markdown
Contributor Author

fabian18 commented Oct 22, 2023

Mhh... to what did you increase the queue size that it overflowed the stack? Does it make sense to increase the stack size as well then?

I increased the size of the IPv6 message queue to 2⁶ to make sure that my problem of an address not getting VALID sometimes is related to that. I don´t want to increase the size upstream. I only want to give the opportunity.
I would like it if stack size and message queue size are independent. I would say stack size scales with code complexity and message queue size scales with IPC communication. Since NIB is posting messages for every interface to IPv6 for doing DAD, router/neighbor discovery, there is a lot of IPC communication.

The problem with adding the queue to static memory for all modules is that this effectively increases the RAM usage of GNRC. Does it make sense to do this only for IPv6 for now?

Having the queue not static and configurable is a bad pattern in my eyes.
Given sizeof(msg_t) == 8 and someone used 6LO, IPv6, UDP, each one having a default queue size of 8, it increases RAM usage by 192 bytes.

You should then reduce the stack size of all GNRC threads by the default queue size to avoid increasing the memory requirements.

#ifndef GNRC_IPV6_STACK_SIZE
#define GNRC_IPV6_STACK_SIZE           (THREAD_STACKSIZE_DEFAULT)
#endif

#ifndef GNRC_PKTDUMP_STACKSIZE
#define GNRC_PKTDUMP_STACKSIZE         (THREAD_STACKSIZE_MAIN)
#endif

#ifndef GNRC_UDP_STACK_SIZE
#define GNRC_UDP_STACK_SIZE            (THREAD_STACKSIZE_SMALL)
#endif

#ifndef GNRC_SIXLOWPAN_STACK_SIZE
#define GNRC_SIXLOWPAN_STACK_SIZE      (THREAD_STACKSIZE_DEFAULT)
#endif

Just subtract 64 bytes there, or is there anything better? If you want, yes but it does not look so nice and someone may wonder why.

@fabian18 fabian18 force-pushed the gnrc_static_messge_queues branch from 7bc7f15 to 85e3988 Compare October 22, 2023 09:39
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Nov 7, 2023 •
edited
Loading

I think subtracting 64 bytes from each of those defines is not too bad, I ended up doing the same for GNRC_NETIF_STACKSIZE_DEFAULT for the same reason. (#17905)

We are lucky that the size of msg_t is architecture independent.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 7, 2023

We are lucky that the size of msg_t is architecture independent.

I think there is less luck and more authorial intend involved ;-)

benpicco
benpicco approved these changes Dec 3, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@maribu maribu added this pull request to the merge queue Dec 4, 2023
Merged via the queue into RIOT-OS:master with commit d252eb7 Dec 4, 2023
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
@fabian18 fabian18 deleted the gnrc_static_messge_queues branch January 11, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@benpicco benpicco benpicco approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

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

Assignees

No one assigned

Labels

Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2024.01

Development

Successfully merging this pull request may close these issues.

6 participants

@fabian18 @riot-ci @miri64 @benpicco @maribu @MrKevinWeiss