calculate_totalreserve_pages() currently finds the maximum
lowmem_reserve[j] for a zone by scanning the full range
[j = zone_idx .. MAX_NR_ZONES). However,
setup_per_zone_lowmem_reserve() constructs lowmem_reserve[]
monotonically increasing in j for a fixed zone (and never populates
lowmem_reserve[zone_idx] itself). This means the maximum valid reserve
entry always resides at the highest j > zone_idx that has a non-zero value.

Rewrite the loop to walk backwards from MAX_NR_ZONES - 1 down to
zone_idx + 1, stopping at the first non-zero lowmem_reserve[j]. This
avoids a full-range scan and makes the intent clearer. Behavior remains
unchanged.

Although this code is not on a hot path, the revised form is clearer
and avoids an unnecessary full scan.

Signed-off-by: fujunjie <fujunjie1@qq.com>
---
 mm/page_alloc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 600d9e981c23d..414c5ba978418 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6285,10 +6285,22 @@ static void calculate_totalreserve_pages(void)
 			long max = 0;
 			unsigned long managed_pages = zone_managed_pages(zone);
 
-			/* Find valid and maximum lowmem_reserve in the zone */
-			for (j = i; j < MAX_NR_ZONES; j++)
-				max = max(max, zone->lowmem_reserve[j]);
+			/*
+			 * Find valid and maximum lowmem_reserve in the zone.
+			 *
+			 * setup_per_zone_lowmem_reserve() builds
+			 * lowmem_reserve[j] monotonically increasing in j
+			 * for a fixed zone, so the maximum lives at the
+			 * highest index that has a non-zero value.  Walk
+			 * backwards and stop at the first hit.
+			 */
+			for (j = MAX_NR_ZONES - 1; j > i; j--) {
+				if (!zone->lowmem_reserve[j])
+					continue;
 
+				max = zone->lowmem_reserve[j];
+				break;
+			}
 			/* we treat the high watermark as reserved pages. */
 			max += high_wmark_pages(zone);
 
-- 
2.34.1

On Fri Nov 14, 2025 at 10:40 AM UTC, fujunjie wrote:
> Although this code is not on a hot path, the revised form is clearer

Is it...?

If people do think it is clearer, let's at least write the right comment
in the right place. Instead of having one piece of code
(calculate_totalreserve_pages()) describe at a distance the behaviour of
another piece of code (setup_per_zone_lowmem_reserve()), let's describe
an invariant of the data ("lowmem_reserve is monotonic, up to the first
zero value"), at the site where the data is defined.

I know sometimes in code this complex we do need these
"spooky-action-at-a-distance" comments but this doesn't seem like one of
those places to me.

On 14 Nov 2025, at 7:36, Brendan Jackman wrote:

> On Fri Nov 14, 2025 at 10:40 AM UTC, fujunjie wrote:
>> Although this code is not on a hot path, the revised form is clearer
>
> Is it...?
>
> If people do think it is clearer, let's at least write the right comment
> in the right place. Instead of having one piece of code
> (calculate_totalreserve_pages()) describe at a distance the behaviour of
> another piece of code (setup_per_zone_lowmem_reserve()), let's describe
> an invariant of the data ("lowmem_reserve is monotonic, up to the first
> zero value"), at the site where the data is defined.

I agree.

>
> I know sometimes in code this complex we do need these
> "spooky-action-at-a-distance" comments but this doesn't seem like one of
> those places to me.

My concern on this change is that the correctness of
calculate_totalreserve_pages() now relies on the implementation of
setup_per_zone_lowmem_reserve(). How can we make sure in the future
this will not break when setup_per_zone_lowmem_reserve() is changed?
Hoping people read the comment and do the right thing?

Best Regards,
Yan, Zi

On Sat Nov 15, 2025 at 00:12 AM UTC, Zi Yan wrote:

> My concern on this change is that the correctness of
> calculate_totalreserve_pages() now relies on the implementation of
> setup_per_zone_lowmem_reserve(). How can we make sure in the future
> this will not break when setup_per_zone_lowmem_reserve() is changed?
> Hoping people read the comment and do the right thing?
Thanks for raising this, Zi.

I agree it would be a real problem if calculate_totalreserve_pages()
were relying on a fragile detail of how setup_per_zone_lowmem_reserve()
happens to be written today.

What I intended to rely on is not an implementation detail, but the
semantics of zone->lowmem_reserve[j] for a given zone (with
zone_idx(zone) == i).

For such a zone "i", zone->lowmem_reserve[j] (j > i) represents how many
pages in zone "i" must effectively be kept in reserve when deciding
whether an allocation class that is allowed to allocate from zones up to
"j" may fall back into zone "i". The purpose of these reserves is to
protect allocation classes that cannot use higher zones and therefore
depend more heavily on this lower zone.

When viewed this way, the partial ordering in j comes from the meaning
of the field: as j increases, we are considering allocation classes that
can use a strictly larger set of fallback zones. Those more flexible
allocations should not be allowed to consume more low memory than the
less flexible ones. It would be quite unexpected—in terms of the reserve
semantics—if a higher-j allocation class were permitted to deplete zone
"i" more aggressively than a lower-j one.

So the “non-decreasing in j” property is really a data invariant implied
by the reserve semantics, rather than an assumption about how
setup_per_zone_lowmem_reserve() happens to be implemented today.

setup_per_zone_lowmem_reserve() currently encodes this meaning by
accumulating managed pages from higher zones and applying the configured
ratio. If some future change were to alter that implementation in a way
that breaks monotonicity, that would likely reflect a change in the
intended semantics of lowmem_reserve itself—at which point consumers
like calculate_totalreserve_pages() would naturally need to be updated
as well.

Best Regards,
Junjie,Fu

On 14 Nov 2025, at 11:34, Fujunjie wrote:

> On Sat Nov 15, 2025 at 00:12 AM UTC, Zi Yan wrote:
>
>> My concern on this change is that the correctness of
>> calculate_totalreserve_pages() now relies on the implementation of
>> setup_per_zone_lowmem_reserve(). How can we make sure in the future
>> this will not break when setup_per_zone_lowmem_reserve() is changed?
>> Hoping people read the comment and do the right thing?
> Thanks for raising this, Zi.
>
> I agree it would be a real problem if calculate_totalreserve_pages()
> were relying on a fragile detail of how setup_per_zone_lowmem_reserve()
> happens to be written today.
>
> What I intended to rely on is not an implementation detail, but the
> semantics of zone->lowmem_reserve[j] for a given zone (with
> zone_idx(zone) == i).
>
> For such a zone "i", zone->lowmem_reserve[j] (j > i) represents how many
> pages in zone "i" must effectively be kept in reserve when deciding
> whether an allocation class that is allowed to allocate from zones up to
> "j" may fall back into zone "i". The purpose of these reserves is to
> protect allocation classes that cannot use higher zones and therefore
> depend more heavily on this lower zone.
>
> When viewed this way, the partial ordering in j comes from the meaning
> of the field: as j increases, we are considering allocation classes that
> can use a strictly larger set of fallback zones. Those more flexible
> allocations should not be allowed to consume more low memory than the
> less flexible ones. It would be quite unexpected—in terms of the reserve
> semantics—if a higher-j allocation class were permitted to deplete zone
> "i" more aggressively than a lower-j one.
>
> So the “non-decreasing in j” property is really a data invariant implied
> by the reserve semantics, rather than an assumption about how
> setup_per_zone_lowmem_reserve() happens to be implemented today.
>
> setup_per_zone_lowmem_reserve() currently encodes this meaning by
> accumulating managed pages from higher zones and applying the configured
> ratio. If some future change were to alter that implementation in a way
> that breaks monotonicity, that would likely reflect a change in the
> intended semantics of lowmem_reserve itself—at which point consumers
> like calculate_totalreserve_pages() would naturally need to be updated
> as well.

Thank you for the explanation. Now your changes make more sense to me.
Like Brendan mentioned, at least add a comment in
setup_per_zone_lowmem_reserve() to state this monotonicity requirement
and mention the correctness of calculate_totalreserve_pages() relies on
it. And also please add the above text to the commit log to clarify
the purpose of the patch.

Thanks.

Best Regards,
Yan, Zi