From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1ocSB1-0007rx-Ao for mharc-grub-devel@gnu.org; Sun, 25 Sep 2022 10:00:11 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:49396) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ocSAw-0007qp-9t for grub-devel@gnu.org; Sun, 25 Sep 2022 10:00:06 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:37251) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ocSAq-0002iP-FZ for grub-devel@gnu.org; Sun, 25 Sep 2022 10:00:06 -0400 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 8236D5C0092; Sun, 25 Sep 2022 09:59:57 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Sun, 25 Sep 2022 09:59:57 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm2; t=1664114397; x=1664200797; bh=D8FBEAwW/O k0wy9v74jHt+VwT+ISfKQymVcqpF9taJw=; b=XNbd3Mt5vvJJDlmFM4L7+rqBWi +6I4ATpSK0zLsgDyaY/5/kOi4bPJeUp7vWt5kTwRY1sqdjfnBspRUlix4OJ9CuLH 03ILpo5JbldVATn5pcKApc+08Fm49bNplTY9noltl1M+Oaf27YpBNchxQIp4g4Ca pkZLKTPqs1oiFRrIPac9UEl3fH/cSECqkEMmgBlNW2d7LuVbswOHS1k2VveP/LWG W39fPTxpSAaQlR9lnlCpaX/OrqOXom61oPbVviF1Q84CuC4/1PvF0U4zLOsROFep 2qDmR/bdAkr22pmda683swKueaCofQFBmbxvQUn3pPgbEXbh6xcMgpSZ4eeg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm2; t=1664114397; x=1664200797; bh=D8FBEAwW/Ok0wy9v74jHt+VwT+IS fKQymVcqpF9taJw=; b=zuZU88bKo7bJy+cWIW6s51qBAYDnfL0hlCUKYm8dGyfZ ot5h6lRjGjLw3rvluTIDe6UY/tuOq6anke+u3phX0GUrPrzsm8OoAyXKU7vE85W6 33nve0bVWkzOo3uRz8n9BL3HQOfaAPQa8F7LrgpZGtBGM+NB1mm/12yHbOOLpyOe b3tiT7HFOzVitRhzAbEplZ1jeEZbj9FgZdwYmeoPXNPQAcpt9B01eSYJJFtxtp4s MPxsuKnhNwTjVRXw5yvDxyHCc1eTmVzJOzwN2AyqPM982jFw7G1GCnDqCKujev7h FYWtdM0QVnqubEjTBSP9YxcvyC5znb0n+6zovEreaw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrfeegtddgjedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpeffhffvvefukfhfgggtuggjsehgtd erredttddvnecuhfhrohhmpefrrghtrhhitghkucfuthgvihhnhhgrrhguthcuoehpshes phhkshdrihhmqeenucggtffrrghtthgvrhhnpeeukedtvedtffevleejtefgheehieegke eluddvfeefgeehgfeltddtheejleffteenucevlhhushhtvghrufhiiigvpedtnecurfgr rhgrmhepmhgrihhlfhhrohhmpehpshesphhkshdrihhm X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 25 Sep 2022 09:59:54 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 118c659e (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 25 Sep 2022 13:59:51 +0000 (UTC) Date: Sun, 25 Sep 2022 15:59:50 +0200 From: Patrick Steinhardt To: Zhang Boyang Cc: grub-devel@gnu.org, jim945@mail.ru, glin@suse.com, dkiper@net-space.pl, dja@axtens.net, droidbittin@gmail.com, heinrich.schuchardt@canonical.com, langner.marcel@myiq.de, marcan@marcan.st Subject: Re: [PATCH v2 1/1] mm: Better handling of adding new regions Message-ID: References: <20220912174952.3046-1-zhangboyang.id@gmail.com> <20220912174952.3046-2-zhangboyang.id@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uDc/XEzd5xSE1c96" Content-Disposition: inline In-Reply-To: <20220912174952.3046-2-zhangboyang.id@gmail.com> Received-SPF: pass client-ip=66.111.4.29; envelope-from=ps@pks.im; helo=out5-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, 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.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 25 Sep 2022 14:00:06 -0000 --uDc/XEzd5xSE1c96 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 13, 2022 at 01:49:52AM +0800, Zhang Boyang wrote: > The code of dynamically adding new regions has two problems. First, it > always invalidate disk caches, which decreases performance severely. > Second, it request exactly "size" bytes for new region, ignoring region > management overheads. >=20 > This patch makes adding new regions more priority than disk cache > invalidating. This patch also use "size + align + GRUB_MM_HEAP_GROW" as > the size of new region. GRUB_MM_HEAP_GROW is set to 1MiB. This value can > address the region overheads, and it can also improve the performance of > small allocations when default heap is full. >=20 > Fixes: 887f98f0db43 (mm: Allow dynamically requesting additional memory r= egions) It might be sensible to split this up into two patches, one to change when we drop caches and one to round requested sizes more intelligently. > Signed-off-by: Zhang Boyang > --- > grub-core/kern/mm.c | 27 +++++++++++++++------------ > include/grub/mm.h | 2 ++ > 2 files changed, 17 insertions(+), 12 deletions(-) >=20 > diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c > index 75f6eacbe..0836b9538 100644 > --- a/grub-core/kern/mm.c > +++ b/grub-core/kern/mm.c > @@ -410,18 +410,21 @@ grub_memalign (grub_size_t align, grub_size_t size) > { > grub_mm_region_t r; > grub_size_t n =3D ((size + GRUB_MM_ALIGN - 1) >> GRUB_MM_ALIGN_LOG2) += 1; > + grub_size_t grow; > int count =3D 0; > =20 > if (!grub_mm_base) > goto fail; > =20 > - if (size > ~(grub_size_t) align) > + if (size > ~(grub_size_t) align || > + (size + align) > ~(grub_size_t) GRUB_MM_HEAP_GROW) > goto fail; > =20 > /* We currently assume at least a 32-bit grub_size_t, > - so limiting allocations to - 1MiB > + so limiting heap growth to - 1MiB > in name of sanity is beneficial. */ > - if ((size + align) > ~(grub_size_t) 0x100000) > + grow =3D size + align + GRUB_MM_HEAP_GROW; > + if (grow > ~(grub_size_t) 0x100000) > goto fail; I wonder whether we want to be a bit more intelligent. It feels like the wrong thing to do to always add 1MB to the request regardless of the requested size. It is probably sensible for small requests, but when you request hundreds of megabytes adding a single megabyte feels rather worthless to me. Maybe we could use some kind of buckets instead, e.g.: - Up to 256kB: allocate 1MB. - Up to 2048kB: allocate 8MB. - Up to 16MB: allocate 64MB. I just make up these numbers, but they should help demonstrate what I mean. > align =3D (align >> GRUB_MM_ALIGN_LOG2); > @@ -443,22 +446,16 @@ grub_memalign (grub_size_t align, grub_size_t size) > switch (count) > { > case 0: > - /* Invalidate disk caches. */ > - grub_disk_cache_invalidate_all (); > - count++; > - goto again; > - > - case 1: It feels sensible to reverse the order here so that we end up trying to satisfy allocations by requesting new pages first. So only when we get into the situation where we really cannot satisfy the request we try to reclaim memory as a last-effort strategy. Patrick > /* Request additional pages, contiguous */ > count++; > =20 > if (grub_mm_add_region_fn !=3D NULL && > - grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_CONSECUTIVE) = =3D=3D GRUB_ERR_NONE) > + grub_mm_add_region_fn (grow, GRUB_MM_ADD_REGION_CONSECUTIVE) = =3D=3D GRUB_ERR_NONE) > goto again; > =20 > /* fallthrough */ > =20 > - case 2: > + case 1: > /* Request additional pages, anything at all */ > count++; > =20 > @@ -468,12 +465,18 @@ grub_memalign (grub_size_t align, grub_size_t size) > * Try again even if this fails, in case it was able to partia= lly > * satisfy the request > */ > - grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_NONE); > + grub_mm_add_region_fn (grow, GRUB_MM_ADD_REGION_NONE); > goto again; > } > =20 > /* fallthrough */ > + case 2: > + /* Invalidate disk caches. */ > + grub_disk_cache_invalidate_all (); > + count++; > + goto again; > + > default: > break; > } > diff --git a/include/grub/mm.h b/include/grub/mm.h > index f3bf87fa0..463096cd8 100644 > --- a/include/grub/mm.h > +++ b/include/grub/mm.h > @@ -29,6 +29,8 @@ > # define NULL ((void *) 0) > #endif > =20 > +#define GRUB_MM_HEAP_GROW 0x100000 > + > #define GRUB_MM_ADD_REGION_NONE 0 > #define GRUB_MM_ADD_REGION_CONSECUTIVE (1 << 0) > =20 > --=20 > 2.30.2 >=20 --uDc/XEzd5xSE1c96 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmMwXtUACgkQVbJhu7ck PpSE7w//QmCxg6yT+4qnM42F4VmXwWmldbeOHCAvWZLGCr3zCIUAOnL4qdLyKQzP HdlLSakFBqVkbUJHxLmD5eFFoaDXhcsiU9wmVcGeqQqLC+7MCr3FDEcVp6O9QPDb wu4sR2+N6nI3g4YsRMUw+a4PdyCrE/OivqW5CtJ+fB9dRJzKKzpp77IqrfLzyfHy cfV/iwrBzBHYDllEI90Tj7hdJFWXLEfIytVg+XEzGNiuO7+w/hrAi2yvhHbaZJFm lwzUfBJsZvByFHFUPQjLpCqBesrphZrZqUkAO5Oba4MwvCjd8+exPbOCwv3nEEi3 Dhg4sMMIIQuVXsy0z9rJBPAZoVF+1em1mcJq0X8qyv95A35qiDQj0MyBGywAw93B yTTKCv/cVmCy+ssyHKyQzxllfp5yUooljU3n48ZHXoyRVuJddMAnKfr2e1yv8oHB LkL8zstOsDyHHJzylWo9qacuWK2PKlTkCnZ+sXJKVcxXwGF8OSJuJt696UqYWCFw OHuNQOxYSXTBm7yQahA+Yf8E0PXSxWaF4JpiAkhUhu3QIi3pEyRsxFyq/LpnZIYK 5/25no1t0pxf3ng7CV3yxpHghSfmZ5obgF9/wFAN8O+pgSWUdOXFNvTvAASv9eTt jCuCpkCiLzr/a7Ygk029kmxkAevHIT9JK0YKeuDVNQyMxqTBU1c= =R+0j -----END PGP SIGNATURE----- --uDc/XEzd5xSE1c96--