mm: page_alloc: unreserve highatomic page blocks before oom

  __alloc_pages_direct_reclaim() is called from slowpath allocation where
high atomic reserves can be unreserved after there is a progress in
reclaim and yet no suitable page is found. Later should_reclaim_retry()
gets called from slow path allocation to decide if the reclaim needs to
be retried before OOM kill path is taken.

should_reclaim_retry() checks the available(reclaimable + free pages)
memory against the min wmark levels of a zone and returns:
a)  true, if it is above the min wmark so that slow path allocation will
do the reclaim retries.
b) false, thus slowpath allocation takes oom kill path.

should_reclaim_retry() can also unreserves the high atomic reserves
**but only after all the reclaim retries are exhausted.**

In a case where there are almost none reclaimable memory and free pages
contains mostly the high atomic reserves but allocation context can't
use these high atomic reserves, makes the available memory below min
wmark levels hence false is returned from should_reclaim_retry() leading
the allocation request to take OOM kill path. This is an early oom kill
because high atomic reserves are holding lot of free memory and 
unreserving of them is not attempted.

(early)OOM is encountered on a machine in the below state(excerpt from
the oom kill logs):
[  295.998653] Normal free:7728kB boost:0kB min:804kB low:1004kB
high:1204kB reserved_highatomic:8192KB active_anon:4kB inactive_anon:0kB
active_file:24kB inactive_file:24kB unevictable:1220kB writepending:0kB
present:70732kB managed:49224kB mlocked:0kB bounce:0kB free_pcp:688kB
local_pcp:492kB free_cma:0kB
[  295.998656] lowmem_reserve[]: 0 32
[  295.998659] Normal: 508*4kB (UMEH) 241*8kB (UMEH) 143*16kB (UMEH)
33*32kB (UH) 7*64kB (UH) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
0*4096kB = 7752kB

Per above log, the free memory of ~7MB exist in the high atomic
reserves is not freed up before falling back to oom kill path.

This fix includes unreserving these atomic reserves in the OOM path
before going for a kill. The side effect of unreserving in oom kill path
is that these free pages are checked against the high wmark. If
unreserved from should_reclaim_retry()/__alloc_pages_direct_reclaim(),
they are checked against the min wmark levels.

Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
---
 mm/page_alloc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
  

On Mon, Oct 30, 2023 at 06:09:50PM +0530, Charan Teja Kalla wrote:
> __alloc_pages_direct_reclaim() is called from slowpath allocation where
> high atomic reserves can be unreserved after there is a progress in
> reclaim and yet no suitable page is found. Later should_reclaim_retry()
> gets called from slow path allocation to decide if the reclaim needs to
> be retried before OOM kill path is taken.
> 
> should_reclaim_retry() checks the available(reclaimable + free pages)
> memory against the min wmark levels of a zone and returns:
> a)  true, if it is above the min wmark so that slow path allocation will
> do the reclaim retries.
> b) false, thus slowpath allocation takes oom kill path.
> 
> should_reclaim_retry() can also unreserves the high atomic reserves
> **but only after all the reclaim retries are exhausted.**
> 
> In a case where there are almost none reclaimable memory and free pages
> contains mostly the high atomic reserves but allocation context can't
> use these high atomic reserves, makes the available memory below min
> wmark levels hence false is returned from should_reclaim_retry() leading
> the allocation request to take OOM kill path. This is an early oom kill
> because high atomic reserves are holding lot of free memory and 
> unreserving of them is not attempted.
> 
> (early)OOM is encountered on a machine in the below state(excerpt from
> the oom kill logs):
> [  295.998653] Normal free:7728kB boost:0kB min:804kB low:1004kB
> high:1204kB reserved_highatomic:8192KB active_anon:4kB inactive_anon:0kB
> active_file:24kB inactive_file:24kB unevictable:1220kB writepending:0kB
> present:70732kB managed:49224kB mlocked:0kB bounce:0kB free_pcp:688kB
> local_pcp:492kB free_cma:0kB
> [  295.998656] lowmem_reserve[]: 0 32
> [  295.998659] Normal: 508*4kB (UMEH) 241*8kB (UMEH) 143*16kB (UMEH)
> 33*32kB (UH) 7*64kB (UH) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
> 0*4096kB = 7752kB
> 
> Per above log, the free memory of ~7MB exist in the high atomic
> reserves is not freed up before falling back to oom kill path.
> 
> This fix includes unreserving these atomic reserves in the OOM path
> before going for a kill. The side effect of unreserving in oom kill path
> is that these free pages are checked against the high wmark. If
> unreserved from should_reclaim_retry()/__alloc_pages_direct_reclaim(),
> they are checked against the min wmark levels.
> 
> Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>

Thanks for the detailed commit description. Really helpful in
understanding the problem you are fixing.

> ---
>  mm/page_alloc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 95546f3..2a2536d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3281,6 +3281,8 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  		.order = order,
>  	};
>  	struct page *page;
> +	struct zone *zone;
> +	struct zoneref *z;
>  
>  	*did_some_progress = 0;
>  
> @@ -3295,6 +3297,16 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  	}
>  
>  	/*
> +	 * If should_reclaim_retry() encounters a state where:
> +	 * reclaimable + free doesn't satisfy the wmark levels,
> +	 * it can directly jump to OOM without even unreserving
> +	 * the highatomic page blocks. Try them for once here
> +	 * before jumping to OOM.
> +	 */
> +retry:
> +	unreserve_highatomic_pageblock(ac, true);
> +

Not possible to fix this in should_reclaim_retry()? 

> +	/*
>  	 * Go through the zonelist yet one more time, keep very high watermark
>  	 * here, this is only to catch a parallel oom killing, we must fail if
>  	 * we're still under heavy pressure. But make sure that this reclaim
> @@ -3307,6 +3319,12 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  	if (page)
>  		goto out;
>  
> +	for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->highest_zoneidx,
> +								ac->nodemask) {
> +		if (zone->nr_reserved_highatomic > 0)
> +			goto retry;
> +	}
> +
>  	/* Coredumps can quickly deplete all memory reserves */
>  	if (current->flags & PF_DUMPCORE)
>  		goto out;
  

On Mon 30-10-23 18:09:50, Charan Teja Kalla wrote:
> __alloc_pages_direct_reclaim() is called from slowpath allocation where
> high atomic reserves can be unreserved after there is a progress in
> reclaim and yet no suitable page is found. Later should_reclaim_retry()
> gets called from slow path allocation to decide if the reclaim needs to
> be retried before OOM kill path is taken.
> 
> should_reclaim_retry() checks the available(reclaimable + free pages)
> memory against the min wmark levels of a zone and returns:
> a)  true, if it is above the min wmark so that slow path allocation will
> do the reclaim retries.
> b) false, thus slowpath allocation takes oom kill path.
> 
> should_reclaim_retry() can also unreserves the high atomic reserves
> **but only after all the reclaim retries are exhausted.**
> 
> In a case where there are almost none reclaimable memory and free pages
> contains mostly the high atomic reserves but allocation context can't
> use these high atomic reserves, makes the available memory below min
> wmark levels hence false is returned from should_reclaim_retry() leading
> the allocation request to take OOM kill path. This is an early oom kill
> because high atomic reserves are holding lot of free memory and 
> unreserving of them is not attempted.

OK, I see. So we do not release those reserved pages because OOM hits
too early. 

> (early)OOM is encountered on a machine in the below state(excerpt from
> the oom kill logs):
> [  295.998653] Normal free:7728kB boost:0kB min:804kB low:1004kB
> high:1204kB reserved_highatomic:8192KB active_anon:4kB inactive_anon:0kB
> active_file:24kB inactive_file:24kB unevictable:1220kB writepending:0kB
> present:70732kB managed:49224kB mlocked:0kB bounce:0kB free_pcp:688kB
> local_pcp:492kB free_cma:0kB
> [  295.998656] lowmem_reserve[]: 0 32
> [  295.998659] Normal: 508*4kB (UMEH) 241*8kB (UMEH) 143*16kB (UMEH)
> 33*32kB (UH) 7*64kB (UH) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
> 0*4096kB = 7752kB

OK, this is quite interesting as well. The system is really tiny and 8MB
of reserved memory is indeed really high. How come those reservations
have grown that high?
> 
> Per above log, the free memory of ~7MB exist in the high atomic
> reserves is not freed up before falling back to oom kill path.
> 
> This fix includes unreserving these atomic reserves in the OOM path
> before going for a kill. The side effect of unreserving in oom kill path
> is that these free pages are checked against the high wmark. If
> unreserved from should_reclaim_retry()/__alloc_pages_direct_reclaim(),
> they are checked against the min wmark levels.

I do not like the fix much TBH. I think the logic should live in
should_reclaim_retry. One way to approach it is to unreserve at the end
of the function, something like this:
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 95546f376302..d04e14adf2c5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3813,10 +3813,8 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 	 * Make sure we converge to OOM if we cannot make any progress
 	 * several times in the row.
 	 */
-	if (*no_progress_loops > MAX_RECLAIM_RETRIES) {
-		/* Before OOM, exhaust highatomic_reserve */
-		return unreserve_highatomic_pageblock(ac, true);
-	}
+	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
+		goto out;
 
 	/*
 	 * Keep reclaiming pages while there is a chance this will lead
@@ -3859,6 +3857,12 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 		schedule_timeout_uninterruptible(1);
 	else
 		cond_resched();
+
+out:
+	/* Before OOM, exhaust highatomic_reserve */
+	if (!ret)
+		return unreserve_highatomic_pageblock(ac, true);
+
 	return ret;
 }
  

Thanks Michal/Pavan!!

On 10/31/2023 1:44 PM, Michal Hocko wrote:
> On Mon 30-10-23 18:09:50, Charan Teja Kalla wrote:
>> __alloc_pages_direct_reclaim() is called from slowpath allocation where
>> high atomic reserves can be unreserved after there is a progress in
>> reclaim and yet no suitable page is found. Later should_reclaim_retry()
>> gets called from slow path allocation to decide if the reclaim needs to
>> be retried before OOM kill path is taken.
>>
>> should_reclaim_retry() checks the available(reclaimable + free pages)
>> memory against the min wmark levels of a zone and returns:
>> a)  true, if it is above the min wmark so that slow path allocation will
>> do the reclaim retries.
>> b) false, thus slowpath allocation takes oom kill path.
>>
>> should_reclaim_retry() can also unreserves the high atomic reserves
>> **but only after all the reclaim retries are exhausted.**
>>
>> In a case where there are almost none reclaimable memory and free pages
>> contains mostly the high atomic reserves but allocation context can't
>> use these high atomic reserves, makes the available memory below min
>> wmark levels hence false is returned from should_reclaim_retry() leading
>> the allocation request to take OOM kill path. This is an early oom kill
>> because high atomic reserves are holding lot of free memory and 
>> unreserving of them is not attempted.
> 
> OK, I see. So we do not release those reserved pages because OOM hits
> too early. 
> 
>> (early)OOM is encountered on a machine in the below state(excerpt from
>> the oom kill logs):
>> [  295.998653] Normal free:7728kB boost:0kB min:804kB low:1004kB
>> high:1204kB reserved_highatomic:8192KB active_anon:4kB inactive_anon:0kB
>> active_file:24kB inactive_file:24kB unevictable:1220kB writepending:0kB
>> present:70732kB managed:49224kB mlocked:0kB bounce:0kB free_pcp:688kB
>> local_pcp:492kB free_cma:0kB
>> [  295.998656] lowmem_reserve[]: 0 32
>> [  295.998659] Normal: 508*4kB (UMEH) 241*8kB (UMEH) 143*16kB (UMEH)
>> 33*32kB (UH) 7*64kB (UH) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
>> 0*4096kB = 7752kB
> 
> OK, this is quite interesting as well. The system is really tiny and 8MB
> of reserved memory is indeed really high. How come those reservations
> have grown that high?

Actually it is a VM running on the Linux kernel.

Regarding the reservations, I think it is because of the 'max_managed '
calculations in the below:
static void reserve_highatomic_pageblock(struct page *page, ....) {
    ....
  /*
   * Limit the number reserved to 1 pageblock or roughly 1% of a zone.
   * Check is race-prone but harmless.
   */
    max_managed = (zone_managed_pages(zone) / 100) + pageblock_nr_pages;

    if (zone->nr_reserved_highatomic >= max_managed)
            goto out;

    zone->nr_reserved_highatomic += pageblock_nr_pages;
    set_pageblock_migratetype(page, MIGRATE_HIGHATOMIC);
    move_freepages_block(zone, page, MIGRATE_HIGHATOMIC, NULL);
out:
}

Since we are always appending the 1% of zone managed pages count to
pageblock_nr_pages, the minimum it is turning into 2 pageblocks as the
'nr_reserved_highatomic' is incremented/decremented in pageblock size
granules.

And for my case the 8M out of ~50M is turned out to be 16%, which is high.

If the below looks fine to you, I can raise this as a separate change:
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2a2536d..41441ced 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1886,7 +1886,9 @@ static void reserve_highatomic_pageblock(struct
page *page, struct zone *zone)
         * Limit the number reserved to 1 pageblock or roughly 1% of a zone.
         * Check is race-prone but harmless.
         */
-       max_managed = (zone_managed_pages(zone) / 100) + pageblock_nr_pages;
+       max_managed = max_t(unsigned long,
+                       ALIGN(zone_managed_pages(zone) / 100,
pageblock_nr_pages),
+                       pageblock_nr_pages);
        if (zone->nr_reserved_highatomic >= max_managed)
                return;

>>
>> Per above log, the free memory of ~7MB exist in the high atomic
>> reserves is not freed up before falling back to oom kill path.
>>
>> This fix includes unreserving these atomic reserves in the OOM path
>> before going for a kill. The side effect of unreserving in oom kill path
>> is that these free pages are checked against the high wmark. If
>> unreserved from should_reclaim_retry()/__alloc_pages_direct_reclaim(),
>> they are checked against the min wmark levels.
> 
> I do not like the fix much TBH. I think the logic should live in

yeah, This code looks way too cleaner to me. Let me know If I can raise
V2 with the below, suggested-by you.

I think another thing system is missing here is draining the pcp lists.
min:804kB low:1004kB high:1204kB free_pcp:688kB

IIUC, the drain pages is being called in reclaim path as below. In this
case, when did_some_progress  = 0, it is also skipping the pcp drain.
struct page *__alloc_pages_direct_reclaim() {
    .....
   *did_some_progress = __perform_reclaim(gfp_mask, order, ac);
   if (unlikely(!(*did_some_progress)))
      goto out;
retry:
    page = get_page_from_freelist();
    if (!page && !drained) {
        drain_all_pages(NULL);
        drained = true;
        goto retry;
    }
out:
}

so, how about the extending the below code from you for this case.
Assuming that did_some_progress > 0 means the draining perhaps already
done in __alloc_pages_direct_reclaim() thus:
out:
   if (!ret) {
       ret = unreserve_highatomic_pageblock(ac, true);
       drain_all_pages(NULL);
   }
   return ret;

Please suggest If the above doesn't make sense. If Looks good, I will
raise a separate patch for this condition.
> should_reclaim_retry. One way to approach it is to unreserve at the end
> of the function, something like this:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 95546f376302..d04e14adf2c5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3813,10 +3813,8 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  	 * Make sure we converge to OOM if we cannot make any progress
>  	 * several times in the row.
>  	 */
> -	if (*no_progress_loops > MAX_RECLAIM_RETRIES) {
> -		/* Before OOM, exhaust highatomic_reserve */
> -		return unreserve_highatomic_pageblock(ac, true);
> -	}
> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
> +		goto out;
>  
>  	/*
>  	 * Keep reclaiming pages while there is a chance this will lead
> @@ -3859,6 +3857,12 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		schedule_timeout_uninterruptible(1);
>  	else
>  		cond_resched();
> +
> +out:
> +	/* Before OOM, exhaust highatomic_reserve */
> +	if (!ret)
> +		return unreserve_highatomic_pageblock(ac, true);
> +
>  	return ret;
>  }
>
  

On Tue 31-10-23 18:43:55, Charan Teja Kalla wrote:
> Thanks Michal/Pavan!!
> 
> On 10/31/2023 1:44 PM, Michal Hocko wrote:
> > On Mon 30-10-23 18:09:50, Charan Teja Kalla wrote:
> >> __alloc_pages_direct_reclaim() is called from slowpath allocation where
> >> high atomic reserves can be unreserved after there is a progress in
> >> reclaim and yet no suitable page is found. Later should_reclaim_retry()
> >> gets called from slow path allocation to decide if the reclaim needs to
> >> be retried before OOM kill path is taken.
> >>
> >> should_reclaim_retry() checks the available(reclaimable + free pages)
> >> memory against the min wmark levels of a zone and returns:
> >> a)  true, if it is above the min wmark so that slow path allocation will
> >> do the reclaim retries.
> >> b) false, thus slowpath allocation takes oom kill path.
> >>
> >> should_reclaim_retry() can also unreserves the high atomic reserves
> >> **but only after all the reclaim retries are exhausted.**
> >>
> >> In a case where there are almost none reclaimable memory and free pages
> >> contains mostly the high atomic reserves but allocation context can't
> >> use these high atomic reserves, makes the available memory below min
> >> wmark levels hence false is returned from should_reclaim_retry() leading
> >> the allocation request to take OOM kill path. This is an early oom kill
> >> because high atomic reserves are holding lot of free memory and 
> >> unreserving of them is not attempted.
> > 
> > OK, I see. So we do not release those reserved pages because OOM hits
> > too early. 
> > 
> >> (early)OOM is encountered on a machine in the below state(excerpt from
> >> the oom kill logs):
> >> [  295.998653] Normal free:7728kB boost:0kB min:804kB low:1004kB
> >> high:1204kB reserved_highatomic:8192KB active_anon:4kB inactive_anon:0kB
> >> active_file:24kB inactive_file:24kB unevictable:1220kB writepending:0kB
> >> present:70732kB managed:49224kB mlocked:0kB bounce:0kB free_pcp:688kB
> >> local_pcp:492kB free_cma:0kB
> >> [  295.998656] lowmem_reserve[]: 0 32
> >> [  295.998659] Normal: 508*4kB (UMEH) 241*8kB (UMEH) 143*16kB (UMEH)
> >> 33*32kB (UH) 7*64kB (UH) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
> >> 0*4096kB = 7752kB
> > 
> > OK, this is quite interesting as well. The system is really tiny and 8MB
> > of reserved memory is indeed really high. How come those reservations
> > have grown that high?
> 
> Actually it is a VM running on the Linux kernel.
> 
> Regarding the reservations, I think it is because of the 'max_managed '
> calculations in the below:
> static void reserve_highatomic_pageblock(struct page *page, ....) {
>     ....
>   /*
>    * Limit the number reserved to 1 pageblock or roughly 1% of a zone.
>    * Check is race-prone but harmless.
>    */
>     max_managed = (zone_managed_pages(zone) / 100) + pageblock_nr_pages;
> 
>     if (zone->nr_reserved_highatomic >= max_managed)
>             goto out;
> 
>     zone->nr_reserved_highatomic += pageblock_nr_pages;
>     set_pageblock_migratetype(page, MIGRATE_HIGHATOMIC);
>     move_freepages_block(zone, page, MIGRATE_HIGHATOMIC, NULL);
> out:
> }
> 
> Since we are always appending the 1% of zone managed pages count to
> pageblock_nr_pages, the minimum it is turning into 2 pageblocks as the
> 'nr_reserved_highatomic' is incremented/decremented in pageblock size
> granules.
> 
> And for my case the 8M out of ~50M is turned out to be 16%, which is high.
> 
> If the below looks fine to you, I can raise this as a separate change:

Yes, please. Having a full page block (4MB) sounds still too much for
such a tiny system. Maybe there shouldn't be any reservation. But
definitely worth a separate patch.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 2a2536d..41441ced 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1886,7 +1886,9 @@ static void reserve_highatomic_pageblock(struct
> page *page, struct zone *zone)
>          * Limit the number reserved to 1 pageblock or roughly 1% of a zone.
>          * Check is race-prone but harmless.
>          */
> -       max_managed = (zone_managed_pages(zone) / 100) + pageblock_nr_pages;
> +       max_managed = max_t(unsigned long,
> +                       ALIGN(zone_managed_pages(zone) / 100,
> pageblock_nr_pages),
> +                       pageblock_nr_pages);
>         if (zone->nr_reserved_highatomic >= max_managed)
>                 return;
> 
> >>
> >> Per above log, the free memory of ~7MB exist in the high atomic
> >> reserves is not freed up before falling back to oom kill path.
> >>
> >> This fix includes unreserving these atomic reserves in the OOM path
> >> before going for a kill. The side effect of unreserving in oom kill path
> >> is that these free pages are checked against the high wmark. If
> >> unreserved from should_reclaim_retry()/__alloc_pages_direct_reclaim(),
> >> they are checked against the min wmark levels.
> > 
> > I do not like the fix much TBH. I think the logic should live in
> 
> yeah, This code looks way too cleaner to me. Let me know If I can raise
> V2 with the below, suggested-by you.

Sure, go ahead.
 
> I think another thing system is missing here is draining the pcp lists.
> min:804kB low:1004kB high:1204kB free_pcp:688kB

Yes, but this seems like negligible even under a small system like that.
Does it actually help to keep system in balance? I would expect that the
OOM is just imminent no matter the draining. Anyway if this makes any
difference then just make it a separate patch please.
  

On Tue, Oct 31, 2023 at 06:43:55PM +0530, Charan Teja Kalla wrote:
> >> (early)OOM is encountered on a machine in the below state(excerpt from
> >> the oom kill logs):
> >> [  295.998653] Normal free:7728kB boost:0kB min:804kB low:1004kB
> >> high:1204kB reserved_highatomic:8192KB active_anon:4kB inactive_anon:0kB
> >> active_file:24kB inactive_file:24kB unevictable:1220kB writepending:0kB
> >> present:70732kB managed:49224kB mlocked:0kB bounce:0kB free_pcp:688kB
> >> local_pcp:492kB free_cma:0kB
> >> [  295.998656] lowmem_reserve[]: 0 32
> >> [  295.998659] Normal: 508*4kB (UMEH) 241*8kB (UMEH) 143*16kB (UMEH)
> >> 33*32kB (UH) 7*64kB (UH) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
> >> 0*4096kB = 7752kB
> > 
> > OK, this is quite interesting as well. The system is really tiny and 8MB
> > of reserved memory is indeed really high. How come those reservations
> > have grown that high?
> 
> Actually it is a VM running on the Linux kernel.
> 
> Regarding the reservations, I think it is because of the 'max_managed '
> calculations in the below:
> static void reserve_highatomic_pageblock(struct page *page, ....) {
>     ....
>   /*
>    * Limit the number reserved to 1 pageblock or roughly 1% of a zone.
>    * Check is race-prone but harmless.
>    */
>     max_managed = (zone_managed_pages(zone) / 100) + pageblock_nr_pages;
> 
>     if (zone->nr_reserved_highatomic >= max_managed)
>             goto out;
> 
>     zone->nr_reserved_highatomic += pageblock_nr_pages;
>     set_pageblock_migratetype(page, MIGRATE_HIGHATOMIC);
>     move_freepages_block(zone, page, MIGRATE_HIGHATOMIC, NULL);
> out:
> }
> 
> Since we are always appending the 1% of zone managed pages count to
> pageblock_nr_pages, the minimum it is turning into 2 pageblocks as the
> 'nr_reserved_highatomic' is incremented/decremented in pageblock size
> granules.
> 
> And for my case the 8M out of ~50M is turned out to be 16%, which is high.
> 
> If the below looks fine to you, I can raise this as a separate change:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 2a2536d..41441ced 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1886,7 +1886,9 @@ static void reserve_highatomic_pageblock(struct
> page *page, struct zone *zone)
>          * Limit the number reserved to 1 pageblock or roughly 1% of a zone.
>          * Check is race-prone but harmless.
>          */
> -       max_managed = (zone_managed_pages(zone) / 100) + pageblock_nr_pages;
> +       max_managed = max_t(unsigned long,
> +                       ALIGN(zone_managed_pages(zone) / 100,
> pageblock_nr_pages),
> +                       pageblock_nr_pages);
>         if (zone->nr_reserved_highatomic >= max_managed)
>                 return;
> 

ALIGN() rounds up the value, so max_t() is not needed here. If you had
used ALIGN_DOWN() then max_t() can be used to keep atleast
pageblock_nr_pages pages.


> >>
> >> Per above log, the free memory of ~7MB exist in the high atomic
> >> reserves is not freed up before falling back to oom kill path.
> >>
> >> This fix includes unreserving these atomic reserves in the OOM path
> >> before going for a kill. The side effect of unreserving in oom kill path
> >> is that these free pages are checked against the high wmark. If
> >> unreserved from should_reclaim_retry()/__alloc_pages_direct_reclaim(),
> >> they are checked against the min wmark levels.
> > 
> > I do not like the fix much TBH. I think the logic should live in
> 
> yeah, This code looks way too cleaner to me. Let me know If I can raise
> V2 with the below, suggested-by you.
> 

Also, add below Fixes tag if it makes sense.

Fixes: 04c8716f7b00 ("mm: try to exhaust highatomic reserve before the OOM")

Thanks,
Pavan
  

On 11/1/2023 12:16 PM, Pavan Kondeti wrote:
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 2a2536d..41441ced 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1886,7 +1886,9 @@ static void reserve_highatomic_pageblock(struct
>> page *page, struct zone *zone)
>>          * Limit the number reserved to 1 pageblock or roughly 1% of a zone.
>>          * Check is race-prone but harmless.
>>          */
>> -       max_managed = (zone_managed_pages(zone) / 100) + pageblock_nr_pages;
>> +       max_managed = max_t(unsigned long,
>> +                       ALIGN(zone_managed_pages(zone) / 100,
>> pageblock_nr_pages),
>> +                       pageblock_nr_pages);
>>         if (zone->nr_reserved_highatomic >= max_managed)
>>                 return;
>>
> ALIGN() rounds up the value, so max_t() is not needed here. If you had
> used ALIGN_DOWN() then max_t() can be used to keep atleast
> pageblock_nr_pages pages.
> 
Yeah, just ALIGN() enough here.
> 
> Also, add below Fixes tag if it makes sense.
> 
> Fixes: 04c8716f7b00 ("mm: try to exhaust highatomic reserve before the OOM")
I should be adding this.
  

On Wed 01-11-23 12:23:24, Charan Teja Kalla wrote:
[...]
> > Also, add below Fixes tag if it makes sense.
> > 
> > Fixes: 04c8716f7b00 ("mm: try to exhaust highatomic reserve before the OOM")
> I should be adding this.

I do not think this Fixes tag is really correct. 04c8716f7b00 was rather
an incomplete fix than something that has caused this situation. I think
we would need to reference the commit which adds highatomic reserves.