| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <D930DO9PAJR8.SOYZSGRG5Y2O@google.com>
Date: Thu, 10 Apr 2025 13:55:27 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Johannes Weiner <hannes@...xchg.org>, Andrew Morton <akpm@...ux-foundation.org>
Cc: Vlastimil Babka <vbabka@...e.cz>, Brendan Jackman <jackmanb@...gle.com>,
Mel Gorman <mgorman@...hsingularity.net>, Carlos Song <carlos.song@....com>, <linux-mm@...ck.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] mm: page_alloc: tighten up find_suitable_fallback()
On Mon Apr 7, 2025 at 6:01 PM UTC, Johannes Weiner wrote:
> find_suitable_fallback() is not as efficient as it could be, and
> somewhat difficult to follow.
>
> 1. should_try_claim_block() is a loop invariant. There is no point in
> checking fallback areas if the caller is interested in claimable
> blocks but the order and the migratetype don't allow for that.
>
> 2. __rmqueue_steal() doesn't care about claimability, so it shouldn't
> have to run those tests.
>
> Different callers want different things from this helper:
>
> 1. __compact_finished() scans orders up until it finds a claimable block
> 2. __rmqueue_claim() scans orders down as long as blocks are claimable
> 3. __rmqueue_steal() doesn't care about claimability at all
>
> Move should_try_claim_block() out of the loop. Only test it for the
> two callers who care in the first place. Distinguish "no blocks" from
> "order + mt are not claimable" in the return value; __rmqueue_claim()
> can stop once order becomes unclaimable, __compact_finished() can keep
> advancing until order becomes claimable.
Nice!
My initial thought was: now we can drop the boolean arg and just have
the callers who care about claimability just call
should_try_claim_block() themselves. Then we can also get rid of the
magic -2 return value and find_suitable_fallback() becomes a pretty
obvious function.
I think it's a win on balance but it does make more verbosity at the
callsites, and an extra interface between page_alloc.c and compaction.c
So maybe it's a wash, maybe you already considered it and decided you
don't prefer it.
So, LGTM either way, I will attempt to attach the optional additional
patch for your perusal, hopefully without molesting the mail encoding
this time...
Reviewed-by: Brendan Jackman <jackmanb@...gle.com>
---
>From 25f77012674db95354fb2496bc89954b8f8b4e6c Mon Sep 17 00:00:00 2001
From: Brendan Jackman <jackmanb@...gle.com>
Date: Thu, 10 Apr 2025 13:22:58 +0000
Subject: [PATCH] mm: page_alloc: Split up find_suitable_fallback()
Now that it's been simplified, it's clear that the bool arg isn't
needed, callers can just use should_try_claim_block(). Once that logic
is stripped out, the function becomes very obvious and can get a more
straightforward name and comment.
Signed-off-by: Brendan Jackman <jackmanb@...gle.com>
---
mm/compaction.c | 3 ++-
mm/internal.h | 5 +++--
mm/page_alloc.c | 33 +++++++++++++--------------------
3 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 39a4d178dff3c..d735c22e71029 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2363,7 +2363,8 @@ static enum compact_result __compact_finished(struct compact_control *cc)
* Job done if allocation would steal freepages from
* other migratetype buddy lists.
*/
- if (find_suitable_fallback(area, order, migratetype, true) >= 0)
+ if (should_try_claim_block(order, migratetype) &&
+ find_fallback_migratetype(area, order, migratetype) >= 0)
/*
* Movable pages are OK in any pageblock. If we are
* stealing for a non-movable allocation, make sure
diff --git a/mm/internal.h b/mm/internal.h
index 4e0ea83aaf1c8..93a8be54924f4 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -914,8 +914,9 @@ static inline void init_cma_pageblock(struct page *page)
#endif
-int find_suitable_fallback(struct free_area *area, unsigned int order,
- int migratetype, bool claimable);
+int find_fallback_migratetype(struct free_area *area, unsigned int order,
+ int migratetype);
+bool should_try_claim_block(unsigned int order, int start_mt);
static inline bool free_area_empty(struct free_area *area, int migratetype)
{
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0a1f28bf5255c..604cad16e1b5a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2034,7 +2034,7 @@ static inline bool boost_watermark(struct zone *zone)
* try to claim an entire block to satisfy further allocations, instead of
* polluting multiple pageblocks?
*/
-static bool should_try_claim_block(unsigned int order, int start_mt)
+bool should_try_claim_block(unsigned int order, int start_mt)
{
/*
* Leaving this order check is intended, although there is
@@ -2076,20 +2076,12 @@ static bool should_try_claim_block(unsigned int order, int start_mt)
return false;
}
-/*
- * Check whether there is a suitable fallback freepage with requested order.
- * If claimable is true, this function returns fallback_mt only if
- * we would do this whole-block claiming. This would help to reduce
- * fragmentation due to mixed migratetype pages in one pageblock.
- */
-int find_suitable_fallback(struct free_area *area, unsigned int order,
- int migratetype, bool claimable)
+/* Find a fallback migratetype with at least one page of the given order. */
+int find_fallback_migratetype(struct free_area *area, unsigned int order,
+ int migratetype)
{
int i;
- if (claimable && !should_try_claim_block(order, migratetype))
- return -2;
-
if (area->nr_free == 0)
return -1;
@@ -2209,18 +2201,19 @@ __rmqueue_claim(struct zone *zone, int order, int start_migratetype,
*/
for (current_order = MAX_PAGE_ORDER; current_order >= min_order;
--current_order) {
+
+ /* Advanced into orders too low to claim, abort */
+ if (!should_try_claim_block(order, start_migratetype))
+ break;
+
area = &(zone->free_area[current_order]);
- fallback_mt = find_suitable_fallback(area, current_order,
- start_migratetype, true);
+ fallback_mt = find_fallback_migratetype(area, current_order,
+ start_migratetype);
/* No block in that order */
if (fallback_mt == -1)
continue;
- /* Advanced into orders too low to claim, abort */
- if (fallback_mt == -2)
- break;
-
page = get_page_from_free_area(area, fallback_mt);
page = try_to_claim_block(zone, page, current_order, order,
start_migratetype, fallback_mt,
@@ -2249,8 +2242,8 @@ __rmqueue_steal(struct zone *zone, int order, int start_migratetype)
for (current_order = order; current_order < NR_PAGE_ORDERS; current_order++) {
area = &(zone->free_area[current_order]);
- fallback_mt = find_suitable_fallback(area, current_order,
- start_migratetype, false);
+ fallback_mt = find_fallback_migratetype(area, current_order,
+ start_migratetype);
if (fallback_mt == -1)
continue;
base-commit: 0e56a6f04d3b06eafe0000d2e3c3d7c2d554366a
--
2.49.0.504.g3bcea36a83-goog
Powered by blists - more mailing lists