xiaoxiang781216 commented on code in PR #17043:
URL: https://github.com/apache/nuttx/pull/17043#discussion_r2370814299
##########
sched/sched/sched_suspendscheduler.c:
##########
@@ -68,6 +68,12 @@ void nxsched_suspend_scheduler(FAR struct tcb_s *tcb)
return;
}
+#ifdef CONFIG_TASK_STACK_OVERFLOW_CHECK
+ /* Check for stack overflow */
+
+ DEBUGASSERT(!up_check_tcbstack_overflow(tcb));
Review Comment:
> Hi @xiaoxiang781216
>
> 1. **I still cant't find the point to update the api of
`up_check_tcbstack `, `up_check_tcbstack `has it's own meaning, and are used
many placed in the kernel code....
up_check_tcbstack and up_check_tcbstack_overflow is almost same, so why not
share the same code?
> [PR17056](https://github.com/apache/nuttx/pull/17056) will make
kernel code not clear and needs more testing to verify the change is OK....**
The change is same on all arch, and we already test on
sim/arm/arm64/riscv/x64.
>
> 2. But i do believe my patch introduce duplicated check in
implementing `up_check_tcbstack_overflow`, I amended it to implement
`up_check_tcbstack_overflow` with the same low level checking as
`up_check_tcbstack`, I think this is more clear and has no duplicated code...
>
up_check_tcbstack_overflow still dup with up_check_tcbstack.
> 3. Besides, I don't think
[PR17056](https://github.com/apache/nuttx/pull/17056) has more feature
implemented, and in fact it doesn't consider the case when
CONFIG_SCHED_SUSPENDSCHEDULER is not enabled, which my patch considered:
>
>
> ```
> #ifdef CONFIG_SCHED_SUSPENDSCHEDULER
> void nxsched_suspend_scheduler(FAR struct tcb_s *tcb);
> #else
> # ifdef CONFIG_TASK_STACK_OVERFLOW_CHECK
> # define nxsched_suspend_scheduler(tcb) \
> do \
> { \
> DEBUGASSERT(!up_check_tcbstack_overflow(tcb));\
> } \
> while (0)
> # else
> # define nxsched_suspend_scheduler(tcb)
> # endif
> #endif
> ```
>
this is fixed by other patch series, @suoyuanG will provide soon.
> 4. you mentioned `compare sp directly` which i doesn't understand...
is there any need to `compare sp diretly` for stack overflow check?
>
if CONFIG_STACKCHECK_MARGIN is zero, the code could skip poking the stack
value, by assume the biggest stack consumption normally happen on the
thread/task switch.
> 5. also, I think it is easy to add more arch support with my PR,
**just implement `up_check_tcbstack_overflow `in arch will be OK**
but #17056 already add the support to all arch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
