os/mm/mm_heap: Optimize alloc fail recovery operation. #6749
Conversation
ewoodev
force-pushed
the
WORK-250401_add_check_condition_security_level_in_alloc_fail
branch
from
6771b56 to
e04b96b
Compare
os/mm/mm_heap/mm_manage_allocfail.c
Outdated
| @@ -97,6 +99,18 @@ void mm_manage_alloc_fail(struct mm_heap_s *heap, int startidx, int endidx, size | |||
| { | |||
| irqstate_t flags = enter_critical_section(); | |||
|
|
|||
| /* If secure state, do not print memory usage and address infomation */ | |||
There was a problem hiding this comment.
Don't you need to move this after cpu pause (line 119)?
There was a problem hiding this comment.
hmmm... little bit duplicated and missing conditional.
How about below?
void mm_manage_alloc_fail(...)
{
enter_critical_section();
cpu pause all
abort_mode = true;
write reboot reason ();
if (!IS_SECURE_STATE()) { <---- only if is added, rest are existed codes.
heap messages
}
cpu resume all
panic();
leave_critical_section();
}
There was a problem hiding this comment.
we don't need to call cpu pause.
the cpu pause is for only log mixing
There was a problem hiding this comment.
I removed duplicated code.
If security level is high, we must not print memory address and dumps. Therefore, we added secure state checking in mfdbg and head_dbg. However, there are many memory dumps. Even if we add secure state checking to mfdbg adn heap_dbg, checking the secure state may cause delays in recovery when outputting every log lines. Therefore, add to check secure state when the first alloc function is called, and if the secure state is active, do not call any memory printing. Signed-off-by: eunwoo.nam <eunwoo.nam@samsung.com>
ewoodev
force-pushed
the
WORK-250401_add_check_condition_security_level_in_alloc_fail
branch
from
e04b96b to
738fc54
Compare
| @@ -109,12 +109,6 @@ void mm_manage_alloc_fail(struct mm_heap_s *heap, int startidx, int endidx, size | |||
| abort_mode = true; | |||
There was a problem hiding this comment.
When the secure level is high, this is not called. Is it ok?
@r-prabu @kishore-sn Could you review this?
There was a problem hiding this comment.
Not sure about it. With the secure state are we restricting only the console logs or log dump as well? If we are only restricting the console logs then will this change wont affect the logdump to these alloc fail logs?
There was a problem hiding this comment.
If security level is high, we don't print alloc fail log, so, we will call PANIC() immediately.
The setting abort_mode is in the up_assert().
There was a problem hiding this comment.
This is conflict with this commit. The variable has too many usages.
| #ifdef CONFIG_DEBUG_MM_HEAPINFO | ||
| void mm_manage_alloc_fail(struct mm_heap_s *heap, int startidx, int endidx, size_t size, size_t align, int heap_type, mmaddress_t caller) | ||
| #else | ||
| void mm_manage_alloc_fail(struct mm_heap_s *heap, int startidx, int endidx, size_t size, size_t align, int heap_type) | ||
| #endif |
There was a problem hiding this comment.
Regardless of this PR, how about using just one api?
When the config is disabled, we can give null or 0 for additional argu.
@r-prabu @kishore-sn How do you think?
There was a problem hiding this comment.
I think this is a good suggestion so that maintainability will be easy.
There was a problem hiding this comment.
How about raise new PR about this suggestion?
If security level is high, we must not print memory address and dumps. Therefore, we added secure state checking in mfdbg and head_dbg.
However, there are many memory dumps. Even if we add secure state checking to mfdbg adn heap_dbg, checking the secure state may cause delays in recovery when outputting every log lines.
Therefore, add to check secure state when the first alloc function is called, and if the secure state is active, do not call any memory printing.