[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 04/12] x86/boot: introduce module release
- To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Jason Andryuk <jason.andryuk@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
- Date: Wed, 6 Nov 2024 19:48:59 -0500
- Arc-authentication-results: i=1; mx.zohomail.com; dkim=pass header.i=apertussolutions.com; spf=pass smtp.mailfrom=dpsmith@xxxxxxxxxxxxxxxxxxxx; dmarc=pass header.from=<dpsmith@xxxxxxxxxxxxxxxxxxxx>
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1730940543; h=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To; bh=Bjl+qG1q3ezB4Ua6CgqxSOZczqOC9SVgseum+rwTgjw=; b=UQZtrwI8EVbmnrj2XVtNxKkvptcSOmeHp0aj1nQe10sP5twFFRU/9d/v+P8M2LZl+AKdbXBvIT0YVFTqRXIjAcyF/pXMyes8y4GRaxEOzxkUzIp3PAn1e6T+F/gHx6MmyHfb0s4L2A4mnXW4aeDc6hcfcTlPCtmNVWqHyStxBmg=
- Arc-seal: i=1; a=rsa-sha256; t=1730940543; cv=none; d=zohomail.com; s=zohoarc; b=TFuoOlA/jjvPC8K1/xiManxGkV9Zw6/hp03Jd3yPj56GvxDfSYq4Jy+Q0Kr40EnTMCKNzGS3JqadiOFDjSd54F4INTexG0w42CLo6qH7F3LscnMrLGi6tA3Bjh9CrRcV2amOF/nS3uV4QrqXKoV6jD4pc+Pk6IC8oZeTj4vzla0=
- Cc: christopher.w.clark@xxxxxxxxx, stefano.stabellini@xxxxxxx, Jan Beulich <jbeulich@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Delivery-date: Thu, 07 Nov 2024 00:49:18 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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
|
