On 11/6/24 17:46, Andrew Cooper wrote:
On 06/11/2024 10:16 pm, Jason Andryuk wrote:
On 2024-11-02 13:25, Daniel P. Smith wrote:
@@ -2122,6 +2152,11 @@ void asmlinkage __init noreturn __start_xen(void)
if ( !dom0 )
panic("Could not set up DOM0 guest OS\n");
+ /* Check and warn if any modules did not get released */
+ for ( i = 0; i < bi->nr_modules; i++ )
+ if ( !bi->mods[i].released )
+ printk(XENLOG_ERR "Boot module %d not released, memory
leaked", i);
+
Why not release the memory here instead of leaking it?
I feel like the corner case of mapping the dom0 initrd is leading to
this manual approach or releasing modules individually. That logic
then has to be spread around. discard_initial_images() kept the logic
centralized. Maybe just replace the mod_end == 0 special case with a
"don't release me" flag that is checked in discard_initial_images()?
I've been conflicted on this for ages, but I agree.
There are many wonky things with the current arrangement.
First(ish), discard_initial_images() should be named
discard_boot_modules() (if it's saying a separate function).
Second, the individual dom0_build.c's should not be calling this
directly. They're both right at the end of construction, so it would
make far more sense for __start_xen() to do this after create_dom0().
That also avoids needing to export the function.
And yes, as Jason says, the "initrd mapped instead of copied" is a weird
corner case, and the other path manually releasing and also saying
"don't free" is just pointless complexity. I do like the idea of a
"dont_free" flag.
Thoughts?
How about the ref count I suggested in response to Jason?
I know this would be moving back to a more v7-like approach, but I think
the end approach is cleaner.
No worries, the series is looking better each time.
v/r,
dps
