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?
I know this would be moving back to a more v7-like approach, but I think
the end approach is cleaner.
~Andrew
