From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mPNQg-00018j-HH for mharc-grub-devel@gnu.org; Sun, 12 Sep 2021 07:13:46 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:57286) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mPNQf-00018a-Cb for grub-devel@gnu.org; Sun, 12 Sep 2021 07:13:45 -0400 Received: from wout1-smtp.messagingengine.com ([64.147.123.24]:32773) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mPNQc-0003rh-RU for grub-devel@gnu.org; Sun, 12 Sep 2021 07:13:45 -0400 Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.west.internal (Postfix) with ESMTP id 5D87A3200900; Sun, 12 Sep 2021 07:13:39 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Sun, 12 Sep 2021 07:13:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=1T2P2mMIUZQ8db7A8Y9aEjAm0Yu 1weT28G3RD9Ej4b0=; b=BVszqY9E2/ZA0xqWhBtVTXS2QLSy1UqQNxJ6RGtkIye q55NPohI+TLJJ804vqkshp1OhNEDVIxzz2c5hCHmWc/8ZTkGsZbct+EwoqjUIkc/ 1ZRmWP2EwNdKbHJZn4gYjzZRpDANoJI0iQ56Qj7gb5FjCRUz2axkj7RzLzsJtmtv Lc9Wl+1Izi9pto9N2cwQCTQ+Ryo/vdgvdPUUpSfvZIsuurqIOTzBGem7reufX0V7 SrUqRJ2UgjPt9CdFFRCpmfesaik5Uu4D998ZSnt938gdEo1H+wJzmc4i2X1jH292 fkCGoKtSeYlLpdIlP0C5LSHe/pIugeIhRR9w+iSPOKA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=1T2P2m MIUZQ8db7A8Y9aEjAm0Yu1weT28G3RD9Ej4b0=; b=jCK5F1dLesanGqZrrkH3/j x5JsVq8Kf47J4DZ6W95myBd+qk3FqogsT41GBpe85MjBGhIQ/1uDcz8WNvsS1tcn 8kg5Un6RBYEnf1sM79BGw0CcTpsJF0ewb6OzTaZpTxckWHAaNplFDC3wNJ+/Qg7l FcJq1P6OkEz+l/3b8qeLQolzcpDWiCxkImsUOk+Bnp58iLgMaekdvFj00CZEIY0v xwqiICBIxgCk6kKL5oTNm35cXIRFG8fAgLx7qD3EVyIVH5wLGamO+9azxtjPRU+V g8qdQlf3FtHL3CIlukFTmr4p6QQgBVnOHcMPc/L6ab6pJ/+fspFbfRSWpJWUE7vQ == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudeghedgfeehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeehgefhtdefueffheekgfffudelffejtdfhvdejkedthfehvdelgfetgfdvtedthfen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhm X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 12 Sep 2021 07:13:37 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id aa4bdfe4 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 12 Sep 2021 11:13:31 +0000 (UTC) Date: Sun, 12 Sep 2021 13:13:31 +0200 From: Patrick Steinhardt To: Daniel Axtens Cc: Daniel Kiper , grub-devel@gnu.org, Leif Lindholm , Stefan Berger Subject: Re: [PATCH v3 2/6] mm: Allow dynamically requesting additional memory regions Message-ID: References: <3f0ec2a76c1b3aa722efdd540b4d3ecce1789750.1629025332.git.ps@pks.im> <87sfyo5p3b.fsf@dja-thinkpad.axtens.net> <20210902124012.jg672of3psba4ie3@tomti.i.net-space.pl> <871r656e6k.fsf@dja-thinkpad.axtens.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="8iYxuzFgJXYszSLw" Content-Disposition: inline In-Reply-To: <871r656e6k.fsf@dja-thinkpad.axtens.net> Received-SPF: pass client-ip=64.147.123.24; envelope-from=ps@pks.im; helo=wout1-smtp.messagingengine.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_PASS=-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: Sun, 12 Sep 2021 11:13:45 -0000 --8iYxuzFgJXYszSLw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 03, 2021 at 10:23:15PM +1000, Daniel Axtens wrote: > Daniel Kiper writes: >=20 > > On Thu, Sep 02, 2021 at 12:48:24AM +1000, Daniel Axtens wrote: > >> Patrick Steinhardt writes: > >> > >> > Currently, all platforms will set up their heap on initialization of= the > >> > platform code. While this works mostly fine, it poses some limitatio= ns > >> > on memory management on us. Most notably, allocating big chunks of > >> > memory in the gigabyte range would require us to pre-request this ma= ny > >> > bytes from the firmware and add it to the heap from the beginning on > >> > some platforms like EFI. As this isn't needed for most configuration= s, > >> > it is inefficient and may even negatively impact some usecases when, > >> > e.g., chainloading. Nonetheless, allocating big chunks of memory is > >> > required sometimes, where one example is the upcoming support for the > >> > Argon2 key derival function in LUKS2. > >> > > >> > In order to avoid pre-allocating big chunks of memory, this commit > >> > implements a runtime mechanism to add more pages to the system. When= a > >> > given allocation cannot be currently satisfied, we'll call a given > >> > callback set up by the platform's own memory management subsystem, > >> > asking it to add a memory area with at least `n` bytes. If this > >> > succeeds, we retry searching for a valid memory region, which should= now > >> > succeed. > >> > > >> > >> I implemented this for ieee1275-powerpc. I set the initial memory claim > >> to 1MB to match EFI and to exercise the code. > >> > >> Thoughts as I progressed: > >> > >> - You probably need to think about how to satisfy requests with > >> particular alignments: currently there is no way to specify that wi= th > >> the current interface, and I saw powerpc-ieee1275 return bunch of > >> allocations at e.g 0x2a561e which is not particularly well aligned! > > > > I think at "firmware memory allocation" level we could always allocate > > page aligned regions. Of course this may not satisfy allocations > > aligned at larger values than a page. Though I think we can solve this > > by allocating larger regions from firmware. Please look below for more > > details... > > > >> - You haven't included in the calculations the extra space required f= or > >> mm housekeeping. For example, I'm seeing an allocation for 32kB be > >> requested via grub_mm_add_region_fn. I dutifully allocate 32kB, with > >> no alignment requirements, and pass that pointer to > >> grub_mm_init_region. grub_mm_init_region throws away bytes at the > >> start to get to GRUB_MM_ALIGN, then uses some bytes for the > >> grub_mm_header_t, then any actual allocation uses bytes for the > >> malloc metadata. So the actual underlying allocation cannot be > >> satisfied. > >> > >> I think you get away with this on EFI because you use BYTES_TO_PAGES > >> and get page-aligned memory, but I think you should probably round = up > >> to the next power of 2 for smaller allocations or to the next page = or > >> so for larger allocations. > > > > I think we could allocate at least e.g. 128 MiB from firmware if there = is > > not enough memory available in the GRUB mm. This way we would avoid fre= quent > > calls to firmware and could satisfy requests for larger alignments. > > > >> - After fixing that in the ieee1275 code, all_functional_test > >> hangs trying to run the cmdline_cat test. I think this is from a sl= ow > >> algorithm somewhere - the grub allocator isn't exactly optimised for > >> a proliferation of regions. > > > > Could you try the solution proposed above? Maybe it will solve problem = of > > frequent additions of memory to the GRUB mm. > > > >> - I noticed that nearly all the allocations were under 1MB. This seems > >> inefficient for a trip out to firmware. So I made the ieee1275 code > >> allocate at least max(4MB, (size of allocation rounded up nearest > >> 1MB) + 4kB). This makes the tests run with only the usual failures, > >> at least on pseries with debug on... still chasing some bugs beyond > >> that. > > > > Yeah, this is similar to what I proposed above. Though I would want to = see > > larger numbers tested as I said earlier. > > > >> - The speed impact depends on the allocation size. I'll post something > >> on that tomorrow, hopefully, but larger minimum allocations work > >> noticably better. > >> > >> - We only have 4GB max to play with because (at least) powerpc-ieee12= 75 > >> is technically defined to be 32 bit. So I'm a bit nervous about > >> further large allocations unless we have a way to release them back > >> to _firmware_, not just to grub. > > > > Ugh... This can be difficult. I am not sure the GRUB mm is smart enough > > to release memory regions if they are not used anymore by it. >=20 > I didn't explain this well. What I'm trying to say is this: >=20 > Say you require 1GB of memory temporarily. >=20 > You do this: >=20 > { > void *mem =3D grub_large_malloc(1024 * 1024 * 1024); > operate_upon(mem); > grub_large_free(mem); > } >=20 > large_malloc and large_free go directly to firmware. We bypass the > existing grub mm infrastructure completely. This way, people who need > this sort of very large allocation do it in a way that can return the > memory to firmware. >=20 > The reason I like this approach more than integrating the memory into > the grub mm infrastructure is that it would be very possible to get > yourself into an unbootable situation by requesting so much memory from > firmware that there's no memory left to load the kernel and initrd into. > In the case of powerpc-ieee1275 there's another failure mode where can > load the kernel but then the kernel can't claim memory from FW for it's > purposes (this is before it quiesces openfirmware and takes control of > memory management itself) and so fails to boot. >=20 > I'll try your suggestions around 128MB allocations next week - I think > they will mostly work but I'm just a bit worried about the implications of > letting grub take arbitrarily large chunks of memory. >=20 > Kind regards, > Daniel To be frank, I don't really mind whether we integrate it into mm directly or make those large allocations explicit: both work equally well for my usecase of allocating memory for memory-hard KDFs. So I'd rather defer the decision to you folks to judge which approach will suit GRUB better in the long term. If we decide to continue with my approach, then I'll try to implement the feedback provided by Daniel, which is to: - Use chunk sizes such that we do not have many tiny allocations, but instead amortize costs (e.g. 32MB chunks). - Have a look at supporting memory alignments. - Account for the mm overhead. But I'll postpone sending another version until we have reached an agreement of where we want to go. Patrick --8iYxuzFgJXYszSLw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmE94NoACgkQVbJhu7ck PpQM1Q//UalLyMlCf/dQELL/q5Ed5g5UFq5zGbOZpdOal+HtnIgXfuS/jOUJ+vqC hTymVnmuhNslHqEKyotVxf3ZUIEF2CLQ+DuagzqvooS7ZPvgf4GH/H4fonvohl4K AGREsZrkFhxaM+gsqZZ312l1TN1eG3ClvE98YCpU+vFXTtD8e+bDBEX/kX7A3FrQ r+jp57KsBx5mHKo2A97MGBfk1b80qT/Cpv7nMczak6IoIwx4R6LrTQviYCXbpEmx nlXkZMfmLS0qmChtdeAa1+mxSqh3Ep6MTFFJzl6TShzhR6ZsPDmYduoY/56CmC0a 36RDegQPRSzBzYnzw9KVlhY9VtGz2ie57MIIRQouvs4uPBzeLZNT1hrOmi5VXq4c B4AYh6zqtZaDAUx8ckVyAdryDHxxWQXAD51cWl9G392YJHPFlBab+JgxhXvFT7ef 6rWfO7WixKvFEhne+CXO+k6jRZJyCWlA3SwpPqbEMd+yBO6GSWGW3N+tqpBKPegF 6Yexsyz+/OX8F8Vs3RiKRxmz3rv3lzSVZhbRmG9oov7rcqWLFINlcfkzSc1cLgSL bLWrlAgvszEbsxDcCv6KM207R1NP5RDjsF9C9ctlzEE0fvxlAzZYBnfYoukvNgza uerfdSfkrVIQilmgWutuBCIFODBCF5HFydl13L8qffiYWSqjlWo= =pUSR -----END PGP SIGNATURE----- --8iYxuzFgJXYszSLw--