Skip to content

core/thread: always use THREAD_CREATE_STACKTEST when DEVELHELP is enabled#20450

Merged
benpicco merged 3 commits intoRIOT-OS:masterfrom
benpicco:THREAD_CREATE_STACKTEST-delete
Jul 29, 2024
Merged

core/thread: always use THREAD_CREATE_STACKTEST when DEVELHELP is enabled#20450
benpicco merged 3 commits intoRIOT-OS:masterfrom
benpicco:THREAD_CREATE_STACKTEST-delete

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Mar 7, 2024 •
edited
Loading

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

@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: pkg Area: External package ports Area: BLE Area: Bluetooth Low Energy support Area: LoRa Area: LoRa radio support Area: OTA Area: Over-the-air updates Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports Area: USB Area: Universal Serial Bus Area: sys Area: System Area: examples Area: Example Applications labels Mar 7, 2024
@benpicco benpicco added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 7, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Mar 7, 2024 •
edited
Loading

Murdock results

✔️ PASSED

0fbc10f core/thread: introduce THREAD_CREATE_NO_STACKTEST

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

Artifacts

@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 7, 2024

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

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Mar 8, 2024 •
edited
Loading

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

#18448 (comment)

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

@benpicco benpicco force-pushed the THREAD_CREATE_STACKTEST-delete branch from ff8eaec to 8f974b8 Compare June 4, 2024 14:23
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jul 18, 2024

For that special case I could add a new THREAD_CREATE_NO_STACKTEST flag

@maribu
Copy link
Copy Markdown
Member

maribu commented Jul 18, 2024

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

benpicco
benpicco commented Jul 18, 2024
View reviewed changes
@benpicco benpicco force-pushed the THREAD_CREATE_STACKTEST-delete branch 2 times, most recently from 992dfe5 to b23d3f5 Compare July 18, 2024 14:44
mguetschow
mguetschow requested changes Jul 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mguetschow mguetschow 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, thanks! What's the reason not to deprecate the flag and removing it in, e.g., a year from now?

@benpicco benpicco force-pushed the THREAD_CREATE_STACKTEST-delete branch from b23d3f5 to 0fbc10f Compare July 29, 2024 09:46
mguetschow
mguetschow approved these changes Jul 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks!

@benpicco benpicco added this pull request to the merge queue Jul 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 29, 2024
@benpicco benpicco added this pull request to the merge queue Jul 29, 2024
Merged via the queue into RIOT-OS:master with commit 41204c8 Jul 29, 2024
@benpicco benpicco deleted the THREAD_CREATE_STACKTEST-delete branch July 29, 2024 15:30
@benpicco benpicco mentioned this pull request Oct 7, 2024
Merged
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mguetschow mguetschow mguetschow approved these changes

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

@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

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

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

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

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

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

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

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

Assignees

No one assigned

Labels

Area: BLE Area: Bluetooth Low Energy support Area: CoAP Area: Constrained Application Protocol implementations Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: LoRa Area: LoRa radio support Area: network Area: Networking Area: OTA Area: Over-the-air updates Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benpicco @riot-ci @maribu @kfessel @mguetschow