RIOT icon indicating copy to clipboard operation
RIOT copied to clipboard

core/thread: always use THREAD_CREATE_STACKTEST when DEVELHELP is enabled

Open benpicco opened this issue 1 year ago • 3 comments

Contribution description

We already create every thread with THREAD_CREATE_STACKTEST. If someone creates a thread without it, ps output is weird (all stack is marked as used) and the overhead is only at thread creation and only when DEVELHELP is set.

Remove the flag (but keep the define to not break apps) since in all those years, not a single use-case has been found for creating a thread without THREAD_CREATE_STACKTEST.

Testing procedure

ps still uses the stack usage with DEVELHELP=1, even when THREAD_CREATE_STACKTEST is not set:

2024-03-07 15:52:26,233 # > ps
2024-03-07 15:52:26,237 # 	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
2024-03-07 15:52:26,238 # 	  - | isr_stack            | -        - |   - |   8192 (   -1) ( 8193) |  0x805f800 |  0x805f800
2024-03-07 15:52:26,238 # 	  1 | idle                 | pending  Q |  15 |   8192 (  436) ( 7756) |  0x8058100 |  0x8059f60 
2024-03-07 15:52:26,238 # 	  2 | main                 | running  Q |   7 |  12288 ( 2776) ( 9512) |  0x805a100 |  0x805cf60 
2024-03-07 15:52:26,239 # 	    | SUM                  |            |     |  28672 ( 3212) (25460)

Issues/PRs references

benpicco avatar Mar 07 '24 14:03 benpicco

Murdock results

:heavy_check_mark: PASSED

0fbc10fb45119ffb615c3ff500ddf2332cb620d6 core/thread: introduce THREAD_CREATE_NO_STACKTEST

Success Failures Total Runtime
10195 0 10196 18m:11s

Artifacts

riot-ci avatar Mar 07 '24 15:03 riot-ci

I think @nmeum had a similar PR open. Maybe it makes sense to revisit that and why that has not been merged?

maribu avatar Mar 07 '24 19:03 maribu

I think @nmeum had a similar PR open. Maybe it makes sense to revisit that and why that has not been merged?

https://github.com/RIOT-OS/RIOT/pull/18448#issuecomment-1216584866

seems like something about short lived threads -- i am not sure how common they are

kfessel avatar Mar 08 '24 17:03 kfessel

For that special case I could add a new THREAD_CREATE_NO_STACKTEST flag

benpicco avatar Jul 18 '24 13:07 benpicco

For that special case I could add a new THREAD_CREATE_NO_STACKTEST flag

👍 But please keep the old flag to explicitly create the stack test defined as 0 around at least for a while. IMO that old flag could even stay, if documented as being a no-op since this has become the default anyway; but I'm also fine with deprecating and phasing out.

(I could see myself explicitly adding the no-op create stack test flag if for some reason the code would rely on that as alternative to lamenting in a comment why the code can safely rely on the stack test being created.)

maribu avatar Jul 18 '24 14:07 maribu