[DNM] fix: prevent virtual heap for multiple initialization #10104
Conversation
this is a fix for 38c4810 It may happen that virtual_heap_init would be called several times. This commit prevents the heap for being re-initialized Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
There was a problem hiding this comment.
Pull Request Overview
A fix to avoid re-initializing the virtual heap when virtual_heap_init is called multiple times.
- Adds a guard to only call
vmh_init_heapon the first invocation - Ensures no error is reported on subsequent invocations if initialization succeeded
| static int virtual_heap_init(void) | ||
| { | ||
| virtual_buffers_heap = vmh_init_heap(&static_hp_buffers, false); | ||
| if (!virtual_buffers_heap) |
There was a problem hiding this comment.
This guard is not thread-safe; concurrent calls may still race and initialize the heap multiple times. Consider using a mutex or atomic compare-and-swap to ensure only one initialization occurs.
There was a problem hiding this comment.
false positive - calls to virtual_heap_init are serialized.
| { 524288, 1} /* buffer for kpb */ | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider adding a brief comment above this function explaining the one-time init guard and expected usage to improve maintainability.
| /* | |
| * Initializes the virtual heap if it has not already been initialized. | |
| * The one-time initialization guard ensures that the heap is only | |
| * initialized once during the system's lifetime. This function is | |
| * registered as a SYS_INIT hook to run during the POST_KERNEL phase. | |
| */ |
| static int virtual_heap_init(void) | ||
| { | ||
| virtual_buffers_heap = vmh_init_heap(&static_hp_buffers, false); | ||
| if (!virtual_buffers_heap) |
There was a problem hiding this comment.
false positive - calls to virtual_heap_init are serialized.
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
sorry, don't understand. virtual_heap_init() is only called on initialisations from a SYS_INIT() abstraction. If such calls can indeed be called more than once, then we're misunderstanding how SYS_INIT() works and we have to review and possibly fix other its uses?
|
@lyakh it is possible that we don't understand something... I did not check why it is called couple of times, just fixed a problem here that was causing regression. |
Once per core ? So maybe the init flow needs more |
@marcinszkudlinski sorry, but this seems rather important. I think we should understand this. If it really can happen (be it once per core or anything else), then it's really important to know that to fix our other use-cases. If it cannot happen and you misinterpreted your test results, then we shouldn't add useless code that can only confuse the reader. Can you describe the conditions when you observed this being called multiple times? |
|
@lyakh absolutely we need to understand. And we need merge this PR ASAP - this problem is causing crashes in some scenarios |
Agree - @marcinszkudlinski are you able to create a BUG to track, we can merge this now to WA issue until root cause is identified. |
Ping @marcinszkudlinski @lyakh do we have a fix or can we go with the WA today (and create a BUG) ? |
|
@lgirdwood not really We need to take a closer look to a build system anyway - this PR is NOT fixing and issue. I'm marking it as DNM |
marcinszkudlinski
changed the title
|
@marcinszkudlinski should we close this or you want to re-use the PR with a different fix? |
4 participants
this is a fix for 38c4810
It may happen that virtual_heap_init would be called several times.
This commit prevents the heap for being re-initialized