From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mKlPP-0001G4-5X for mharc-grub-devel@gnu.org; Mon, 30 Aug 2021 13:49:23 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:35106) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mKlPN-0001Fu-Gg for grub-devel@gnu.org; Mon, 30 Aug 2021 13:49:21 -0400 Received: from dibed.net-space.pl ([84.10.22.86]:34360) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1mKlPL-0000iu-Ta for grub-devel@gnu.org; Mon, 30 Aug 2021 13:49:21 -0400 Received: from router-fw.i.net-space.pl ([192.168.52.1]:47462 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2106054AbhH3RtO (ORCPT ); Mon, 30 Aug 2021 19:49:14 +0200 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Mon, 30 Aug 2021 19:49:07 +0200 From: Daniel Kiper To: Daniel Axtens Cc: Patrick Steinhardt , grub-devel@gnu.org, Leif Lindholm , Stefan Berger Subject: Re: [PATCH v3 0/6] Runtime allocation of memory regions Message-ID: <20210830174907.mhvzxxinxeugdhca@tomti.i.net-space.pl> References: <20210826151232.zo2embyicdlpotrz@tomti.i.net-space.pl> <87wno77e06.fsf@dja-thinkpad.axtens.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87wno77e06.fsf@dja-thinkpad.axtens.net> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Aug 2021 17:49:21 -0000 On Fri, Aug 27, 2021 at 01:39:05PM +1000, Daniel Axtens wrote: > Daniel Kiper writes: > > > Hey, > > > > Adding Daniel Axtens... > > > > On Sun, Aug 15, 2021 at 01:09:06PM +0200, Patrick Steinhardt wrote: > >> Hi, > >> > >> this is the third version of my patch set to implement runtime > >> allocation of additional memory regions. > >> > >> Changes compared to v2: > >> > >> - A new preparatory patch was added to remove unused code which > >> unloaded modules on OOM. > >> > >> - Patch 2/4 has been split up into two patches: one to drop the > >> logic where we request a quarter of available memory and then > >> put bounds to it, and one to split apart request of additional > >> regions and initialization of the EFI MM system. > >> > >> - Flags are now `unsigned int` instead of `unsigned`. > >> > >> - `add_memory_regions ()` now gets all flags instead of only a > >> single flag `consecutive`. > >> > >> - Flags are now defines and not an enum anymore. > >> > >> - The callback function is now called `grub_mm_add_region_func_t` > >> instead of `grub_mm_region_func_t`. Flags and its variable have > >> been renamed accordingly. > >> > >> Patrick > >> > >> Patrick Steinhardt (6): > >> mm: Drop unused unloading of modules on OOM > >> mm: Allow dynamically requesting additional memory regions > >> efi: mm: Always request a fixed number of pages on init > >> efi: mm: Extract function to add memory regions > >> efi: mm: Pass up errors from `add_memory_regions ()` > >> efi: mm: Implement runtime addition of pages > >> > >> grub-core/kern/dl.c | 20 ---------- > >> grub-core/kern/efi/mm.c | 83 +++++++++++++++++++---------------------- > >> grub-core/kern/mm.c | 12 +++--- > >> include/grub/dl.h | 1 - > >> include/grub/mm.h | 16 ++++++++ > >> 5 files changed, 61 insertions(+), 71 deletions(-) > > > > Patrick, I went quickly through this patch series and in general it > > LGTM. There are some minor issues but we can fix them later. Thank > > you for doing this work. > > > > Stefan and/or Daniel Axtens, may I ask you to test these patches with > > your use case? If it works for you please repost this patch series with > > your changes added. Then I will merge it after final review. > > Sure, I'll have a look. > > My initial thoughts are: > > - with the CAS support patch. We would still need that, and would want > to do that call early as possible because it will cause the partition > to be rebooted. > > - We have to be careful where we ask for memory because the kernel > assumes that there will be some free memory below a particular address. > > - I'd also want to verify what the performance impact would be - not > just on powerpc-ieee1275 but also on efi - of going out to > OpenFirmware/UEFI for each new zone... Good point! I was thinking about performance at some point too. Sadly I forgot to say something about that... Patrick, did you compare performance before and after you patch series? If the impact is significant maybe we should not change initial allocation strategy and add a function to allocate more memory during runtime only... > I'll test this early next week and report back. Cool! Thanks a lot! Daniel