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

gnrc: make message queues static #19998

Merged
merged 6 commits into from
Dec 4, 2023

Conversation

fabian18
Copy link
Contributor

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

riot-ci commented Oct 19, 2023

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
Member

miri64 commented Oct 20, 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?

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?

core/msg.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

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
Contributor Author

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
Contributor

benpicco commented Nov 7, 2023

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
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 ;-)

Copy link
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
26 checks passed
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants