Add new API to pause and resume the CPUs and handle it properly #6608
Conversation
os/arch/arm/src/armv7-a/arm_assert.c
Outdated
| #ifdef CONFIG_SMP | ||
| /* If SMP is enabled then we need to resume all the other cpu's | ||
| * which we paused earlier. | ||
| */ | ||
| up_cpu_resume_all(); | ||
| #endif |
There was a problem hiding this comment.
Is this right to resume all cpus before recovering the fault?
There was a problem hiding this comment.
So, there will be two cases:
- If it is kernel fault, then the board will reboot. Resuming all CPUs won't affect this case.
- If the crash is in app binary, then we will be pausing all the CPUs inside recovery.
There was a problem hiding this comment.
- Even we call reboot, why do we need to resume?
- Who(where) pauses the all cpus in recovery?
There was a problem hiding this comment.
- Yeah, I agree. There is no point in resuming here for this case.
- As, we are not pausing the CPUs in recovery as of now, let me revert this change.
There was a problem hiding this comment.
- When app fault is done in recovery, all the cpus should be resumed. You should add resuming at somewhere.
There was a problem hiding this comment.
Let me check it properly.
There was a problem hiding this comment.
As of now, we don't support recovery with SMP, let us add it separately while supporting recovery
| * Returned Value: | ||
| * Zero on success; a negated errno value on failure. |
There was a problem hiding this comment.
It always returns OK. Need the return?
There was a problem hiding this comment.
I also think we can make return type void.
There was a problem hiding this comment.
You changed the return type to void, but this line is not changed.
neel-samsung
force-pushed
the
common_pause_api
branch
from
90d6499 to
2fec0cf
Compare
os/mm/mm_heap/mm_manage_allocfail.c
Outdated
| @@ -90,6 +90,14 @@ void mm_manage_alloc_fail(struct mm_heap_s *heap, int startidx, int endidx, size | |||
| { | |||
| irqstate_t flags = enter_critical_section(); | |||
|
|
|||
| #ifdef CONFIG_SMP | |||
| /* If SMP is enabled then we need to pause all the other cpu's immediately | |||
| * because the kernel state might be invalid at this point. If we don't pause | |||
There was a problem hiding this comment.
I dont think the kernel state will be invalid. Let us just mention that we pause the other cpu to avoid mixed logs.
This patch adds new API to pause and resume all the cpus other than current cpu. Signed-off-by: neel-samsung <[email protected]>
…nt log mixup This patch adds code to pause all the CPUs while printing the memory allocation failure logs. It immediately resume the paused cores after printing it. Signed-off-by: neel-samsung <[email protected]>
neel-samsung
force-pushed
the
common_pause_api
branch
from
2fec0cf to
89d00e0
Compare
|
@r-prabu @kishore-sn please review and merge this. |
os/arch/arm/src/armv7-a/arm_assert.c
Outdated
| /* If SMP is enabled and there is a crash, then we need to pause all | ||
| * the other cpu's immediately because the kernel state might be | ||
| * invalid at this point. If we dont pause other cpu's then it might | ||
| * lead to multiple asserts. |
There was a problem hiding this comment.
I think this comment is not valid. Let us change as follows:
"Pause all other cpus to avoid mix up of logs while printing asssert logs"
Earlier, we pause the CPUs only when the crash is in kernel space. But now, regardless of the assert space, we pause all the other CPUs. We also resume the CPUs just after the printing of assert logs. Signed-off-by: neel-samsung <[email protected]>
neel-samsung
force-pushed
the
common_pause_api
branch
from
89d00e0 to
1df3075
Compare
|
@ewoodev Could you let us know how to reproduce the issue? |
|
I havn't reproduct this issue. |
@neel-samsung Could you confirm this PR with the way @ewoodev suggested? |
yes, I have made cpu0 to mem alloc fail and on the other hand created new thread to run on the core1 to print the log. |
|
@kishore-sn @r-prabu Let's merge |
No description provided.