Re: [PATCH 04/12] x86/boot: introduce module release

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-06 19:46, Daniel P. Smith wrote:
> On 11/6/24 17:16, Jason Andryuk wrote:
> > On 2024-11-02 13:25, Daniel P. Smith wrote:
> > > A precarious approach was used to release the pages used to hold a 
> > > boot module.
> > > The precariousness stemmed from the fact that in the case of PV dom0, 
> > > the
> > > initrd module pages may be either mapped or explicitly copied into 
> > > the dom0
> > > address space. So to handle this situation, the PV dom0 construction 
> > > code will
> > > set the size of the module to zero, relying on 
> > > discard_initial_images() to skip
> > > any modules with a size of zero.
> > >
> > > A function is introduced to release a module when it is no longer 
> > > needed that
> > > accepts a boolean parameter, free_mem, to indicate if the 
> > > corresponding pages
> > > can be freed. To track that a module has been released, the boot 
> > > module flag
> > > `released` is introduced.
> > >
> > > The previous release model was a free all at once except those of 
> > > size zeros,
> > > which would handle any unused modules passed. The new model is one 
> > > of, free
> > > consumed module after usage is complete, thus unconsumed modules do 
> > > not have a
> > > consumer to free them.
> > Slightly confusing.  Maybe just "The new model is to free modules 
> > after they are consumed.  Thus unconsumed modules are not freed."
> okay.
>
> > > To address this, the discard_uknown_boot_modules() is
> > "unknown"
> Ack
>
> > > introduced and called after the last module identification occurs, 
> > > initrd, to
> > > free the pages of any boot modules that are identified as not being 
> > > released.
> > > After domain construction completes, all modules should be freed. A 
> > > walk of the
> > > boot modules is added after domain construction to validate and 
> > > notify if a
> > > module is found not to have been released.
> > >
> > > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > +    {
> > > +        struct boot_module *bm = &bi->mods[i];
> > > +
> > > +        if ( bm == NULL || bm->released )
> > >               continue;
> > > -        init_domheap_pages(start, start + PAGE_ALIGN(size));
> > > +        release_boot_module(bm, true);
> > > +        count++;
> > >       }
> > > -    bi->nr_modules = 0;
> > > +    if ( count )
> > > +        printk(XENLOG_DEBUG "Releasing pages for uknown boot module 
> > > %d\n",
> > "unknown".  Since the operation already happened, maybe "Released 
> > pages for %d unknown boot modules"?  I'm not sure of the value of that 
> > message.  It would be more informative to provide the module index and 
> > maybe a page count.  The index would at least point to which module is 
> > unused.
> Ack to unknown.
>
> Can adjust the phrasing, the question is there a desire to have a 
> message for every boot module freed. Guess I could do a single log line 
> split across multiple printks, Thinking about the case where someone 
> tried to abuse the interface by loading a bunch of unused modules.
It's 63 modules, so not too many.  And this is the boot path, so it's 
administrator controlled and shouldn't really be subject to abuse.
I think printing the index of unused modules makes the message useful. 
If a module is provided to Xen, but Xen doesn't use it, it is worth 
flagging which one.  As an example, if I thought module 3 was ucode, but 
it isn't getting used, that is useful information for further investigation.
> > > +               count);
> > >   }
> > >   static void __init init_idle_domain(void)
> > > @@ -2111,6 +2139,8 @@ void asmlinkage __init noreturn __start_xen(void)
> > >                      initrdidx);
> > >       }
> > > +    discard_unknown_boot_modules();
> > > +
> > >       /*
> > >        * We're going to setup domain0 using the module(s) that we 
> > > stashed safely
> > >        * above our heap. The second module, if present, is an initrd 
> > > ramdisk.
> > > @@ -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?
> Because you don't know if it was mapped or consumed.
So this is more of a sanity check since it should not trigger?  i.e. 
it's a bug to see this message.
> > 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()?
> That is what started me at the options to deal with it. The two I came 
> up with was a flag and this approach. Weighing the pros/cons of the two, 
> the deciding factor is when multi-domain construction is introduced. 
> With multi-domain with a large number of domains, a balance has to be 
> struck between holding all the kernels and ramdisks in memory plus being 
> able to allocate the desired amount of memory for each domain.
So you're saying that by piece-wise free-ing memory, you can have more 
domUs loaded by the boot loader?  If free-ing is delayed to the end, 
memory is tied up that could otherwise be assigned to domUs?
Is this the real motivation?  If so, it belongs in the commit message. 
As it is, the commit message is lacking in a specify reason why this 
change is needed.
> Perhaps a 
> balance can be struck, with a discard_consumed_boot_modules() that walks 
> the boot module list, and discard the ones consumed. While only dom0 can 
> be constructed, it would be called once after create_dom0 call returns 
> (per Andy's suggestion in response to this comment). An expansion on 
> this could be that instead of using a free flag, use a ref count that is 
> incremented as it is claimed and the decremented when it has been consumed.
Are you expecting 1 kernel image gets used to create N domUs?  Or are 
you only handling 1:1?
Is mapping the initrd only expected for a single PV dom0, or do you 
envision mapping multiple initrds for multiple PV domUs?
It seems to me that more mapping might help alleviate keeping memory 
tied up.  Freeing XSM and ucode early helps, but those aren't 
particularly big.  An initrd in the 100s of MBs is significant though. 
If you hyperlaunch Xen and 2 domUs, you'd want to be able to assign ~all 
the memory.  Losing 2x ~100MB as unassigned for copies of the initrds is 
wasteful.
Regards,
Jason