Re: [PATCH] Revert "mm/page_alloc.c: don't show protection in zone's ->lowmem_reserve[] for empty zone"

[Michal Hocko <mhocko@suse.com>]

On Tue 25-02-25 22:22:58, Gabriel Krisman Bertazi wrote:
> Commit 96a5c186efff ("mm/page_alloc.c: don't show protection in zone's
> ->lowmem_reserve[] for empty zone") removes the protection of lower
> zones from allocations targeting memory-less high zones.  This had an
> unintended impact on the pattern of reclaims because it makes the
> high-zone-targeted allocation more likely to succeed in lower zones,
> which adds pressure to said zones.  I.e, the following corresponding
> checks in zone_watermark_ok/zone_watermark_fast are less likely to
> trigger:
> 
>         if (free_pages <= min + z->lowmem_reserve[highest_zoneidx])
>                 return false;
> 
> As a result, we are observing an increase in reclaim and kswapd scans,
> due to the increased pressure.  This was initially observed as increased
> latency in filesystem operations when benchmarking with fio on a machine
> with some memory-less zones, but it has since been associated with
> increased contention in locks related to memory reclaim.  By reverting
> this patch, the original performance was recovered on that machine.

I think it would be nice to show the memory layout on that machine (is
there any movable or device zone)?

Exact reclaim patterns are really hard to predict and it is little bit
surprising the said patch has caused an increased kswapd activity
because I would expect that there will be more reclaim with the lowmem
reserves in place. But it is quite possible that the higher zone memory
pressure is just tipping over and increase the lowmem pressure enough
that it shows up.

In any case 96a5c186efff seems incorrect because it assumes that the
protection has anything to do with how higher zone is populated while
the protection fundamentaly protects lower zone from higher zones
allocation. Those allocations are independent on the actual memory in
that zone.

> The original commit was introduced as a clarification of the
> /proc/zoneinfo output, so it doesn't seem there are usecases depending
> on it, making the revert a simple solution.
> 
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Baoquan He <bhe@redhat.com>
> Fixes: 96a5c186efff ("mm/page_alloc.c: don't show protection in zone's ->lowmem_reserve[] for empty zone")
> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

Acked-by: Michal Hocko <mhocko@suse.com>
Thanks!

> ---
>  mm/page_alloc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 579789600a3c..fe986e6de7a0 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5849,11 +5849,10 @@ static void setup_per_zone_lowmem_reserve(void)
>  
>  			for (j = i + 1; j < MAX_NR_ZONES; j++) {
>  				struct zone *upper_zone = &pgdat->node_zones[j];
> -				bool empty = !zone_managed_pages(upper_zone);
>  
>  				managed_pages += zone_managed_pages(upper_zone);
>  
> -				if (clear || empty)
> +				if (clear)
>  					zone->lowmem_reserve[j] = 0;
>  				else
>  					zone->lowmem_reserve[j] = managed_pages / ratio;
> -- 
> 2.47.0

-- 
Michal Hocko
SUSE Labs