Try to retrieve the stack usage from filling on ThreadX - #550
Wang-Perry wants to merge 1 commit into
Conversation
we still have chance to retrieve the stack usage if stack filling is enabled, even stack checking is disabled.
| = (stackHighestPtr >= stackStartPtr && stackHighestPtr <= stackEndPtr); | ||
| ULONG const* current = stackHighestPtrValid ? stackHighestPtr : stackCurrentPtr; | ||
|
|
||
| while (current > stackStartPtr && *current != stackFillWord) |
There was a problem hiding this comment.
Wouldn't it make more sense to flip the fill-word logic? Searching over fill-words until it is not found anymore?
There was a problem hiding this comment.
Yes, that's also my feeling, because the valid data in the stack can be the fill word. I think what Roland proposes is the better heuristic.
There was a problem hiding this comment.
Thanks for the feedback.
We agree that searching from the beginning of the stack for the first non-fill word is more robust, since valid stack data can theoretically contain the fill word.
We actually considered this approach initially. However, this would require scanning the unused part of every thread's stack on each check, which can become expensive when the stacks are relatively large.
The current implementation starts from the previously detected stack usage and searches towards the beginning of the stack. The assumption is that stack usage normally grows incrementally, so this significantly reduces the amount of memory that needs to be scanned on each check.
This does mean that a valid stack value matching the fill pattern could cause the usage to be underestimated. This can be mitigated by using a distinctive fill pattern; ThreadX uses 0xEFEFEFEF by default.
So the current approach is a deliberate trade-off between accuracy and runtime overhead for periodic stack-usage monitoring. We can also document this limitation in the code.
| ULONG const stackFillWord = static_cast<ULONG>(TX_STACK_FILL); | ||
|
|
||
| // Downward stack growth | ||
| if (stackEndPtr >= stackStartPtr) |
There was a problem hiding this comment.
How does this work? I mean is it an OS configuration option which direction the stack grows?
If so, couldn't we use that parameter here? I am very cautious because we are relying with a decision here on values, that in an error case could theoretically be overwritten and corrupted. This could also lead to very long loops later in the code.
There was a problem hiding this comment.
Thanks for the feedback.
We agree that invalid stackStartPtr or stackEndPtr values could result in an invalid search range and potentially an unbounded loop.
For ThreadX, there is no equivalent of FreeRTOS's portSTACK_GROWTH macro that we can use here. ThreadX's default stack-checking logic assumes downward-growing stacks, while the stack-checking implementation can be overridden by individual ports when a different stack direction is required.
Also, the existing OpenBSW stack-filling implementation already relies on the same stackStartPtr and stackEndPtr information to determine the stack range. This PR uses the same mechanism.
Therefore, we consider validating or defining the stack boundaries to be outside the scope of this PR. The current change focuses on measuring the stack usage based on the existing stack boundaries.
There was a problem hiding this comment.
Maybe we should rephrase the commit message because its suggesting sth. non deterministic and that might be worrying in OS related code. Either we have a solution or not :-)
There was a problem hiding this comment.
OK. I agree that the current word "try" can be misleading. How about rephrase the commit message to "Improve stack usage measurement"?
There was a problem hiding this comment.
Agree.
Please update the PR accordingly. As far as I can see, there was no activity from your side on this PR for a month.
| taskHandle.tx_thread_stack_highest_ptr | ||
| = const_cast<VOID*>(reinterpret_cast<VOID const*>(current)); | ||
| } | ||
| else // Upward stack growth |
There was a problem hiding this comment.
I think this "upward stack growth" branch is dead code, since in ThreadX, tx_thread_stack_end = stack_start + size - 1. (With size being unsigned.) Independent of the growth direction.
Same is true for the above pre-existing branch, also a bug.
This branch has not been deployed
4 participants
we still have chance to retrieve the stack usage if stack filling is enabled, even stack checking is disabled.