| 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: <D93WJ0HYKRGL.1NWTMXWJBWE80@google.com>
Date: Fri, 11 Apr 2025 15:07:01 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Vlastimil Babka <vbabka@...e.cz>,
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 Fri Apr 11, 2025 at 1:45 PM UTC, Johannes Weiner wrote:
>> - if (find_suitable_fallback(area, order, migratetype, true) >= 0)
>> + if (should_try_claim_block(order, migratetype) &&
>> + find_fallback_migratetype(area, order, migratetype) >= 0)
>
> So I agree with pushing the test into the callers. However, I think
> the name "should_try_claim_block()" is not great for this. It makes
> sense in the alloc/fallback path, but compaction here doesn't claim
> anything. It just wants to know if this order + migratetype is
> eligible under block claiming rules.
>
> IMO this would be more readable with the old terminology:
>
> if (can_claim_block(order, migratetype) &&
> find_fallback_migratetype(area, order, migratetype) >= 0)
Sure, that makes sense, here's a modified version of the patch:
---
>From 85be0fca4627c5b832a3382c92b6310609e14ca4 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.
Since should_try_claim_block() is now exported to compaction.c, give it
a name that makes more sense outside the context of allocation -
should_claim_block() seems confusing in code that has no interest in
actually claiming a block.
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..0528996c40507 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 (can_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..5450ea7f5b1ec 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 can_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..c27a106ec5985 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 can_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 (!can_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;
--
2.49.0.604.gff1f9ca942-goog
Powered by blists - more mailing lists