Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1497021 > unrolled thread

[PATCH 0/4] use up highorder free pages before OOM

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] use up highorder free pages before OOM Minchan Kim <minchan@kernel.org> - 2016-10-07 07:50 +0200
    [PATCH 4/4] mm: skip to reserve pageblock crossed zone boundary for HIGHATOMIC Minchan Kim <minchan@kernel.org> - 2016-10-07 07:50 +0200
    [PATCH 1/4] mm: adjust reserved highatomic count Minchan Kim <minchan@kernel.org> - 2016-10-07 07:50 +0200
      Re: [PATCH 1/4] mm: adjust reserved highatomic count Vlastimil Babka <vbabka@suse.cz> - 2016-10-07 14:40 +0200
        Re: [PATCH 1/4] mm: adjust reserved highatomic count Minchan Kim <minchan@kernel.org> - 2016-10-07 16:30 +0200
          Re: [PATCH 1/4] mm: adjust reserved highatomic count Vlastimil Babka <vbabka@suse.cz> - 2016-10-10 09:40 +0200
            Re: [PATCH 1/4] mm: adjust reserved highatomic count Minchan Kim <minchan@kernel.org> - 2016-10-11 06:40 +0200
              Re: [PATCH 1/4] mm: adjust reserved highatomic count Vlastimil Babka <vbabka@suse.cz> - 2016-10-11 12:00 +0200
              Re: [PATCH 1/4] mm: adjust reserved highatomic count Mel Gorman <mgorman@techsingularity.net> - 2016-10-12 07:50 +0200
    [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Minchan Kim <minchan@kernel.org> - 2016-10-07 07:50 +0200
      Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-07 11:10 +0200
        Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Minchan Kim <minchan@kernel.org> - 2016-10-07 16:50 +0200
          Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-10 09:50 +0200
            Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Minchan Kim <minchan@kernel.org> - 2016-10-11 07:10 +0200
              Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-11 09:00 +0200
                Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Minchan Kim <minchan@kernel.org> - 2016-10-11 09:30 +0200
                  Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-11 09:30 +0200
                    Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Minchan Kim <minchan@kernel.org> - 2016-10-11 09:40 +0200
                      Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-11 10:10 +0200
    Re: [PATCH 0/4] use up highorder free pages before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-07 11:20 +0200
      Re: [PATCH 0/4] use up highorder free pages before OOM Minchan Kim <minchan@kernel.org> - 2016-10-07 17:50 +0200
        Re: [PATCH 0/4] use up highorder free pages before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-10 10:00 +0200
          Re: [PATCH 0/4] use up highorder free pages before OOM Minchan Kim <minchan@kernel.org> - 2016-10-11 07:10 +0200
            Re: [PATCH 0/4] use up highorder free pages before OOM Michal Hocko <mhocko@kernel.org> - 2016-10-11 09:00 +0200

Page 1 of 2  [1] 2  Next page →

#1497021 — [PATCH 0/4] use up highorder free pages before OOM

I got OOM report from production team with v4.4 kernel.
It has enough free memory but failed to allocate order-0 page and
finally encounter OOM kill.
I could reproduce it with my test easily. Look at below.
The reason is free pages(19M) of DMA32 zone are reserved for
HIGHORDERATOMIC and doesn't unreserved before the OOM.

balloon invoked oom-killer: gfp_mask=0x24280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), order=0, oom_score_adj=0
balloon cpuset=/ mems_allowed=0
CPU: 1 PID: 8473 Comm: balloon Tainted: G        W  OE   4.8.0-rc7-00219-g3f74c9559583-dirty #3161
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
 0000000000000000 ffff88007f15bbc8 ffffffff8138eb13 ffff88007f15bd88
 ffff88005a72a4c0 ffff88007f15bc28 ffffffff811d2d13 ffff88007f15bc08
 ffffffff8146a5ca ffffffff81c8df60 0000000000000015 0000000000000206
Call Trace:
 [<ffffffff8138eb13>] dump_stack+0x63/0x90
 [<ffffffff811d2d13>] dump_header+0x5c/0x1ce
 [<ffffffff8146a5ca>] ? virtballoon_oom_notify+0x2a/0x80
 [<ffffffff81171e5e>] oom_kill_process+0x22e/0x400
 [<ffffffff8117222c>] out_of_memory+0x1ac/0x210
 [<ffffffff811775ce>] __alloc_pages_nodemask+0x101e/0x1040
 [<ffffffff811a245a>] handle_mm_fault+0xa0a/0xbf0
 [<ffffffff8106029d>] __do_page_fault+0x1dd/0x4d0
 [<ffffffff81060653>] trace_do_page_fault+0x43/0x130
 [<ffffffff81059bda>] do_async_page_fault+0x1a/0xa0
 [<ffffffff817a3f38>] async_page_fault+0x28/0x30
Mem-Info:
active_anon:383949 inactive_anon:106724 isolated_anon:0
 active_file:15 inactive_file:44 isolated_file:0
 unevictable:0 dirty:0 writeback:24 unstable:0
 slab_reclaimable:2483 slab_unreclaimable:3326
 mapped:0 shmem:0 pagetables:1906 bounce:0
 free:6898 free_pcp:291 free_cma:0
Node 0 active_anon:1535796kB inactive_anon:426896kB active_file:60kB inactive_file:176kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:0kB dirty:0kB writeback:96kB shmem:0kB writeback_tmp:0kB unstable:0kB pages_scanned:1418 all_unreclaimable? no
DMA free:8188kB min:44kB low:56kB high:68kB active_anon:7648kB inactive_anon:0kB active_file:0kB inactive_file:4kB unevictable:0kB writepending:0kB present:15992kB managed:15908kB mlocked:0kB slab_reclaimable:0kB slab_unreclaimable:20kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
lowmem_reserve[]: 0 1952 1952 1952
DMA32 free:19404kB min:5628kB low:7624kB high:9620kB active_anon:1528148kB inactive_anon:426896kB active_file:60kB inactive_file:420kB unevictable:0kB writepending:96kB present:2080640kB managed:2030092kB mlocked:0kB slab_reclaimable:9932kB slab_unreclaimable:13284kB kernel_stack:2496kB pagetables:7624kB bounce:0kB free_pcp:900kB local_pcp:112kB free_cma:0kB
lowmem_reserve[]: 0 0 0 0
DMA: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 2*4096kB (H) = 8192kB
DMA32: 7*4kB (H) 8*8kB (H) 30*16kB (H) 31*32kB (H) 14*64kB (H) 9*128kB (H) 2*256kB (H) 2*512kB (H) 4*1024kB (H) 5*2048kB (H) 0*4096kB = 19484kB
51131 total pagecache pages
50795 pages in swap cache
Swap cache stats: add 3532405601, delete 3532354806, find 124289150/1822712228
Free swap  = 8kB
Total swap = 255996kB
524158 pages RAM
0 pages HighMem/MovableOnly
12658 pages reserved
0 pages cma reserved
0 pages hwpoisoned

During the investigation, I found some problems with highatomic so
this patch aims to solve the problems and the final goal is to
unreserve every highatomic free pages before the OOM kill.

Patch 1 fixes accounting bug in several places of page allocators
Patch 2 fixes accounting bug caused by subtle race between freeing
function and unreserve_highatomic_pageblock.
Patch 3 changes unreseve scheme to use up every reserved pages
Patch 4 fixes accounting bug caused by mem_section shared by two zones.

Minchan Kim (4):
  mm: adjust reserved highatomic count
  mm: prevent double decrease of nr_reserved_highatomic
  mm: unreserve highatomic free pages fully before OOM
  mm: skip to reserve pageblock crossed zone boundary for HIGHATOMIC

 mm/page_alloc.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 118 insertions(+), 25 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1497022 — [PATCH 4/4] mm: skip to reserve pageblock crossed zone boundary for HIGHATOMIC

In CONFIG_SPARSEMEM, VM shares a pageblock_flags of a mem_section
between two zones if the pageblock cross zone boundaries. It means
a zone lock cannot protect pageblock migratype change's race.

It might be not a problem because migratetype inherently was racy
but intrdocuing with CMA, it was not true any more and have been fixed.
(I hope it should be solved more general approach however...)
And then, it's time for MIGRATE_HIGHATOMIC.

More importantly, HIGHATOMIC migratetype is not big(i.e., 1%) reserve
in system so let's skip such crippled pageblock to try to reserve
full 1% free memory.

Debugged-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/page_alloc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index eeb047bb0e9d..d76bb50baf61 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2098,6 +2098,24 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
 	mt = get_pageblock_migratetype(page);
 	if (mt != MIGRATE_HIGHATOMIC &&
 			!is_migrate_isolate(mt) && !is_migrate_cma(mt)) {
+		/*
+		 * If the pageblock cross zone boundaries, we need both
+		 * zone locks but doesn't want to make complex because
+		 * highatomic pageblock is small so that we want to reserve
+		 * sane(?) pageblock.
+		 */
+		unsigned long start_pfn, end_pfn;
+
+		start_pfn = page_to_pfn(page);
+		start_pfn = start_pfn & ~(pageblock_nr_pages - 1);
+
+		if (!zone_spans_pfn(zone, start_pfn))
+			goto out_unlock;
+
+		end_pfn = start_pfn + pageblock_nr_pages - 1;
+		if (!zone_spans_pfn(zone, end_pfn))
+			goto out_unlock;
+
 		zone->nr_reserved_highatomic += pageblock_nr_pages;
 		set_pageblock_migratetype(page, MIGRATE_HIGHATOMIC);
 		move_freepages_block(zone, page, MIGRATE_HIGHATOMIC);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1497023 — [PATCH 1/4] mm: adjust reserved highatomic count

In page freeing path, migratetype is racy so that a highorderatomic
page could free into non-highorderatomic free list. If that page
is allocated, VM can change the pageblock from higorderatomic to
something. In that case, we should adjust nr_reserved_highatomic.
Otherwise, VM cannot reserve highorderatomic pageblocks any more
although it doesn't reach 1% limit. It means highorder atomic
allocation failure would be higher.

So, this patch decreases the account as well as migratetype
if it was MIGRATE_HIGHATOMIC.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/page_alloc.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 55ad0229ebf3..e7cbb3cc22fa 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -282,6 +282,9 @@ EXPORT_SYMBOL(nr_node_ids);
 EXPORT_SYMBOL(nr_online_nodes);
 #endif
 
+static void dec_highatomic_pageblock(struct zone *zone, struct page *page,
+					int migratetype);
+
 int page_group_by_mobility_disabled __read_mostly;
 
 #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
@@ -1935,7 +1938,14 @@ static void change_pageblock_range(struct page *pageblock_page,
 	int nr_pageblocks = 1 << (start_order - pageblock_order);
 
 	while (nr_pageblocks--) {
-		set_pageblock_migratetype(pageblock_page, migratetype);
+		if (get_pageblock_migratetype(pageblock_page) !=
+			MIGRATE_HIGHATOMIC)
+			set_pageblock_migratetype(pageblock_page,
+							migratetype);
+		else
+			dec_highatomic_pageblock(page_zone(pageblock_page),
+							pageblock_page,
+							migratetype);
 		pageblock_page += pageblock_nr_pages;
 	}
 }
@@ -1996,8 +2006,14 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
 
 	/* Claim the whole block if over half of it is free */
 	if (pages >= (1 << (pageblock_order-1)) ||
-			page_group_by_mobility_disabled)
-		set_pageblock_migratetype(page, start_type);
+			page_group_by_mobility_disabled) {
+		int mt = get_pageblock_migratetype(page);
+
+		if (mt != MIGRATE_HIGHATOMIC)
+			set_pageblock_migratetype(page, start_type);
+		else
+			dec_highatomic_pageblock(zone, page, start_type);
+	}
 }
 
 /*
@@ -2037,6 +2053,17 @@ int find_suitable_fallback(struct free_area *area, unsigned int order,
 	return -1;
 }
 
+static void dec_highatomic_pageblock(struct zone *zone, struct page *page,
+					int migratetype)
+{
+	if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
+		return;
+
+	zone->nr_reserved_highatomic -= min(pageblock_nr_pages,
+					zone->nr_reserved_highatomic);
+	set_pageblock_migratetype(page, migratetype);
+}
+
 /*
  * Reserve a pageblock for exclusive use of high-order atomic allocations if
  * there are no empty page blocks that contain a page with a suitable order
@@ -2555,9 +2582,14 @@ int __isolate_free_page(struct page *page, unsigned int order)
 		struct page *endpage = page + (1 << order) - 1;
 		for (; page < endpage; page += pageblock_nr_pages) {
 			int mt = get_pageblock_migratetype(page);
-			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt))
-				set_pageblock_migratetype(page,
-							  MIGRATE_MOVABLE);
+			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)) {
+				if (mt != MIGRATE_HIGHATOMIC)
+					set_pageblock_migratetype(page,
+							MIGRATE_MOVABLE);
+				else
+					dec_highatomic_pageblock(zone, page,
+							MIGRATE_MOVABLE);
+			}
 		}
 	}
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1497137 — Re: [PATCH 1/4] mm: adjust reserved highatomic count

On 10/07/2016 07:45 AM, Minchan Kim wrote:
> In page freeing path, migratetype is racy so that a highorderatomic
> page could free into non-highorderatomic free list.

Yes. If page from a pageblock went to a pcplist before that pageblock 
was reserved as highatomic, free_pcppages_bulk() will misplace it.

> If that page
> is allocated, VM can change the pageblock from higorderatomic to
> something.

More specifically, steal_suitable_fallback(). Yes.

> In that case, we should adjust nr_reserved_highatomic.
> Otherwise, VM cannot reserve highorderatomic pageblocks any more
> although it doesn't reach 1% limit. It means highorder atomic
> allocation failure would be higher.
>
> So, this patch decreases the account as well as migratetype
> if it was MIGRATE_HIGHATOMIC.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Hm wouldn't it be simpler just to prevent the pageblock's migratetype to 
be changed if it's highatomic? Possibly also not do 
move_freepages_block() in that case. Most accurate would be to put such 
misplaced page on the proper freelist and retry the fallback, but that 
might be overkill.

> ---
>  mm/page_alloc.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 55ad0229ebf3..e7cbb3cc22fa 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -282,6 +282,9 @@ EXPORT_SYMBOL(nr_node_ids);
>  EXPORT_SYMBOL(nr_online_nodes);
>  #endif
>
> +static void dec_highatomic_pageblock(struct zone *zone, struct page *page,
> +					int migratetype);
> +
>  int page_group_by_mobility_disabled __read_mostly;
>
>  #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> @@ -1935,7 +1938,14 @@ static void change_pageblock_range(struct page *pageblock_page,
>  	int nr_pageblocks = 1 << (start_order - pageblock_order);
>
>  	while (nr_pageblocks--) {
> -		set_pageblock_migratetype(pageblock_page, migratetype);
> +		if (get_pageblock_migratetype(pageblock_page) !=
> +			MIGRATE_HIGHATOMIC)
> +			set_pageblock_migratetype(pageblock_page,
> +							migratetype);
> +		else
> +			dec_highatomic_pageblock(page_zone(pageblock_page),
> +							pageblock_page,
> +							migratetype);
>  		pageblock_page += pageblock_nr_pages;
>  	}
>  }
> @@ -1996,8 +2006,14 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
>
>  	/* Claim the whole block if over half of it is free */
>  	if (pages >= (1 << (pageblock_order-1)) ||
> -			page_group_by_mobility_disabled)
> -		set_pageblock_migratetype(page, start_type);
> +			page_group_by_mobility_disabled) {
> +		int mt = get_pageblock_migratetype(page);
> +
> +		if (mt != MIGRATE_HIGHATOMIC)
> +			set_pageblock_migratetype(page, start_type);
> +		else
> +			dec_highatomic_pageblock(zone, page, start_type);
> +	}
>  }
>
>  /*
> @@ -2037,6 +2053,17 @@ int find_suitable_fallback(struct free_area *area, unsigned int order,
>  	return -1;
>  }
>
> +static void dec_highatomic_pageblock(struct zone *zone, struct page *page,
> +					int migratetype)
> +{
> +	if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
> +		return;
> +
> +	zone->nr_reserved_highatomic -= min(pageblock_nr_pages,
> +					zone->nr_reserved_highatomic);
> +	set_pageblock_migratetype(page, migratetype);
> +}
> +
>  /*
>   * Reserve a pageblock for exclusive use of high-order atomic allocations if
>   * there are no empty page blocks that contain a page with a suitable order
> @@ -2555,9 +2582,14 @@ int __isolate_free_page(struct page *page, unsigned int order)
>  		struct page *endpage = page + (1 << order) - 1;
>  		for (; page < endpage; page += pageblock_nr_pages) {
>  			int mt = get_pageblock_migratetype(page);
> -			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt))
> -				set_pageblock_migratetype(page,
> -							  MIGRATE_MOVABLE);
> +			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)) {
> +				if (mt != MIGRATE_HIGHATOMIC)
> +					set_pageblock_migratetype(page,
> +							MIGRATE_MOVABLE);
> +				else
> +					dec_highatomic_pageblock(zone, page,
> +							MIGRATE_MOVABLE);
> +			}
>  		}
>  	}
>
>

[toc] | [prev] | [next] | [standalone]

#1497181 — Re: [PATCH 1/4] mm: adjust reserved highatomic count

Hi Vlastimil,

On Fri, Oct 07, 2016 at 02:30:04PM +0200, Vlastimil Babka wrote:
> On 10/07/2016 07:45 AM, Minchan Kim wrote:
> >In page freeing path, migratetype is racy so that a highorderatomic
> >page could free into non-highorderatomic free list.
> 
> Yes. If page from a pageblock went to a pcplist before that pageblock was
> reserved as highatomic, free_pcppages_bulk() will misplace it.

As well, high-order freeing path has a problem, too.


    CPU 1                               CPU 2
    
                                        __free_pages_ok
                                        /* got highatomic mt */
    unreserve_highatomic_pageblock      mt = get_pfnblock_migratetype
    spin_lock_irqsave(&zone->lock);
    move_freepages_block
    /* change from highatomic to something
    set_pageblock_migratetype(page)
    spin_unlock_irqrestore(&zone->lock)
    
                                        spin_lock(&zone->lock);
                                        /* highatomic mt is stale */
                                        __free_one_page(page, mt);
 
Acutually, I tried to solve this problem with fixing the free path
but it needs to add a branch to verify highorderatomic mt in
both order-0 and high-order page freeing path. On highorder page freeing
path wouldn't be a problem but I don't want to add the branch in pcp
freeing path which is hot.

> 
> >If that page
> >is allocated, VM can change the pageblock from higorderatomic to
> >something.
> 
> More specifically, steal_suitable_fallback(). Yes.

As well, __isolate_free_page, too.

> 
> >In that case, we should adjust nr_reserved_highatomic.
> >Otherwise, VM cannot reserve highorderatomic pageblocks any more
> >although it doesn't reach 1% limit. It means highorder atomic
> >allocation failure would be higher.
> >
> >So, this patch decreases the account as well as migratetype
> >if it was MIGRATE_HIGHATOMIC.
> >
> >Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> Hm wouldn't it be simpler just to prevent the pageblock's migratetype to be
> changed if it's highatomic? Possibly also not do move_freepages_block() in

It could be. Actually, I did it with modifying can_steal_fallback which returns
false it found the pageblock is highorderatomic but changed to this way again
because I don't have any justification to prevent changing pageblock.
If you give concrete justification so others isn't against on it, I am happy to
do what you suggested.

> that case. Most accurate would be to put such misplaced page on the proper
> freelist and retry the fallback, but that might be overkill.
> 
> >---
> > mm/page_alloc.c | 44 ++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 38 insertions(+), 6 deletions(-)
> >
> >diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >index 55ad0229ebf3..e7cbb3cc22fa 100644
> >--- a/mm/page_alloc.c
> >+++ b/mm/page_alloc.c
> >@@ -282,6 +282,9 @@ EXPORT_SYMBOL(nr_node_ids);
> > EXPORT_SYMBOL(nr_online_nodes);
> > #endif
> >
> >+static void dec_highatomic_pageblock(struct zone *zone, struct page *page,
> >+					int migratetype);
> >+
> > int page_group_by_mobility_disabled __read_mostly;
> >
> > #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> >@@ -1935,7 +1938,14 @@ static void change_pageblock_range(struct page *pageblock_page,
> > 	int nr_pageblocks = 1 << (start_order - pageblock_order);
> >
> > 	while (nr_pageblocks--) {
> >-		set_pageblock_migratetype(pageblock_page, migratetype);
> >+		if (get_pageblock_migratetype(pageblock_page) !=
> >+			MIGRATE_HIGHATOMIC)
> >+			set_pageblock_migratetype(pageblock_page,
> >+							migratetype);
> >+		else
> >+			dec_highatomic_pageblock(page_zone(pageblock_page),
> >+							pageblock_page,
> >+							migratetype);
> > 		pageblock_page += pageblock_nr_pages;
> > 	}
> > }
> >@@ -1996,8 +2006,14 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
> >
> > 	/* Claim the whole block if over half of it is free */
> > 	if (pages >= (1 << (pageblock_order-1)) ||
> >-			page_group_by_mobility_disabled)
> >-		set_pageblock_migratetype(page, start_type);
> >+			page_group_by_mobility_disabled) {
> >+		int mt = get_pageblock_migratetype(page);
> >+
> >+		if (mt != MIGRATE_HIGHATOMIC)
> >+			set_pageblock_migratetype(page, start_type);
> >+		else
> >+			dec_highatomic_pageblock(zone, page, start_type);
> >+	}
> > }
> >
> > /*
> >@@ -2037,6 +2053,17 @@ int find_suitable_fallback(struct free_area *area, unsigned int order,
> > 	return -1;
> > }
> >
> >+static void dec_highatomic_pageblock(struct zone *zone, struct page *page,
> >+					int migratetype)
> >+{
> >+	if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
> >+		return;
> >+
> >+	zone->nr_reserved_highatomic -= min(pageblock_nr_pages,
> >+					zone->nr_reserved_highatomic);
> >+	set_pageblock_migratetype(page, migratetype);
> >+}
> >+
> > /*
> >  * Reserve a pageblock for exclusive use of high-order atomic allocations if
> >  * there are no empty page blocks that contain a page with a suitable order
> >@@ -2555,9 +2582,14 @@ int __isolate_free_page(struct page *page, unsigned int order)
> > 		struct page *endpage = page + (1 << order) - 1;
> > 		for (; page < endpage; page += pageblock_nr_pages) {
> > 			int mt = get_pageblock_migratetype(page);
> >-			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt))
> >-				set_pageblock_migratetype(page,
> >-							  MIGRATE_MOVABLE);
> >+			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)) {
> >+				if (mt != MIGRATE_HIGHATOMIC)
> >+					set_pageblock_migratetype(page,
> >+							MIGRATE_MOVABLE);
> >+				else
> >+					dec_highatomic_pageblock(zone, page,
> >+							MIGRATE_MOVABLE);
> >+			}
> > 		}
> > 	}
> >
> >
> 

[toc] | [prev] | [next] | [standalone]

#1498150 — Re: [PATCH 1/4] mm: adjust reserved highatomic count

On 10/07/2016 04:29 PM, Minchan Kim wrote:
>>> In that case, we should adjust nr_reserved_highatomic.
>>> Otherwise, VM cannot reserve highorderatomic pageblocks any more
>>> although it doesn't reach 1% limit. It means highorder atomic
>>> allocation failure would be higher.
>>>
>>> So, this patch decreases the account as well as migratetype
>>> if it was MIGRATE_HIGHATOMIC.
>>>
>>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>>
>> Hm wouldn't it be simpler just to prevent the pageblock's migratetype to be
>> changed if it's highatomic? Possibly also not do move_freepages_block() in
>
> It could be. Actually, I did it with modifying can_steal_fallback which returns
> false it found the pageblock is highorderatomic but changed to this way again
> because I don't have any justification to prevent changing pageblock.
> If you give concrete justification so others isn't against on it, I am happy to
> do what you suggested.

Well, MIGRATE_HIGHATOMIC is not listed in the fallbacks array at all, so 
we are not supposed to steal from it in the first place. Stealing will 
only happen due to races, which would be too costly to close, so we 
allow them and expect to be rare. But we shouldn't allow them to break 
the accounting.

[toc] | [prev] | [next] | [standalone]

#1498603 — Re: [PATCH 1/4] mm: adjust reserved highatomic count

Hi Vlasimil,

On Mon, Oct 10, 2016 at 08:57:40AM +0200, Vlastimil Babka wrote:
> On 10/07/2016 04:29 PM, Minchan Kim wrote:
> >>>In that case, we should adjust nr_reserved_highatomic.
> >>>Otherwise, VM cannot reserve highorderatomic pageblocks any more
> >>>although it doesn't reach 1% limit. It means highorder atomic
> >>>allocation failure would be higher.
> >>>
> >>>So, this patch decreases the account as well as migratetype
> >>>if it was MIGRATE_HIGHATOMIC.
> >>>
> >>>Signed-off-by: Minchan Kim <minchan@kernel.org>
> >>
> >>Hm wouldn't it be simpler just to prevent the pageblock's migratetype to be
> >>changed if it's highatomic? Possibly also not do move_freepages_block() in
> >
> >It could be. Actually, I did it with modifying can_steal_fallback which returns
> >false it found the pageblock is highorderatomic but changed to this way again
> >because I don't have any justification to prevent changing pageblock.
> >If you give concrete justification so others isn't against on it, I am happy to
> >do what you suggested.
> 
> Well, MIGRATE_HIGHATOMIC is not listed in the fallbacks array at all, so we
> are not supposed to steal from it in the first place. Stealing will only
> happen due to races, which would be too costly to close, so we allow them
> and expect to be rare. But we shouldn't allow them to break the accounting.
> 

Fair enough.
How about this?

From 4a0b6a74ebf1af7f90720b0028da49e2e2a2b679 Mon Sep 17 00:00:00 2001
From: Minchan Kim <minchan@kernel.org>
Date: Thu, 6 Oct 2016 13:38:35 +0900
Subject: [PATCH] mm: don't steal highatomic pageblock

In page freeing path, migratetype is racy so that a highorderatomic
page could free into non-highorderatomic free list. If that page
is allocated, VM can change the pageblock from higorderatomic to
something. In that case, highatomic pageblock accounting is broken
so it doesn't work(e.g., VM cannot reserve highorderatomic pageblocks
any more although it doesn't reach 1% limit).

So, this patch prohibits the changing from highatomic to other type.
It's no problem because MIGRATE_HIGHATOMIC is not listed in fallback
array so stealing will only happen due to unexpected races which is
really rare. Also, such prohibiting keeps highatomic pageblock more
longer so it would be better for highorderatomic page allocation.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/page_alloc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 55ad0229ebf3..79853b258211 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2154,7 +2154,8 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
 
 		page = list_first_entry(&area->free_list[fallback_mt],
 						struct page, lru);
-		if (can_steal)
+		if (can_steal &&
+			get_pageblock_migratetype(page) != MIGRATE_HIGHATOMIC)
 			steal_suitable_fallback(zone, page, start_migratetype);
 
 		/* Remove the page from the freelists */
@@ -2555,7 +2556,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
 		struct page *endpage = page + (1 << order) - 1;
 		for (; page < endpage; page += pageblock_nr_pages) {
 			int mt = get_pageblock_migratetype(page);
-			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt))
+			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)
+				&& mt != MIGRATE_HIGHATOMIC)
 				set_pageblock_migratetype(page,
 							  MIGRATE_MOVABLE);
 		}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1498695 — Re: [PATCH 1/4] mm: adjust reserved highatomic count

On 10/11/2016 06:19 AM, Minchan Kim wrote:
> Hi Vlasimil,
>
> On Mon, Oct 10, 2016 at 08:57:40AM +0200, Vlastimil Babka wrote:
>> On 10/07/2016 04:29 PM, Minchan Kim wrote:
>> >>>In that case, we should adjust nr_reserved_highatomic.
>> >>>Otherwise, VM cannot reserve highorderatomic pageblocks any more
>> >>>although it doesn't reach 1% limit. It means highorder atomic
>> >>>allocation failure would be higher.
>> >>>
>> >>>So, this patch decreases the account as well as migratetype
>> >>>if it was MIGRATE_HIGHATOMIC.
>> >>>
>> >>>Signed-off-by: Minchan Kim <minchan@kernel.org>
>> >>
>> >>Hm wouldn't it be simpler just to prevent the pageblock's migratetype to be
>> >>changed if it's highatomic? Possibly also not do move_freepages_block() in
>> >
>> >It could be. Actually, I did it with modifying can_steal_fallback which returns
>> >false it found the pageblock is highorderatomic but changed to this way again
>> >because I don't have any justification to prevent changing pageblock.
>> >If you give concrete justification so others isn't against on it, I am happy to
>> >do what you suggested.
>>
>> Well, MIGRATE_HIGHATOMIC is not listed in the fallbacks array at all, so we
>> are not supposed to steal from it in the first place. Stealing will only
>> happen due to races, which would be too costly to close, so we allow them
>> and expect to be rare. But we shouldn't allow them to break the accounting.
>>
>
> Fair enough.
> How about this?

Look sgood.

> From 4a0b6a74ebf1af7f90720b0028da49e2e2a2b679 Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Thu, 6 Oct 2016 13:38:35 +0900
> Subject: [PATCH] mm: don't steal highatomic pageblock
>
> In page freeing path, migratetype is racy so that a highorderatomic
> page could free into non-highorderatomic free list. If that page
> is allocated, VM can change the pageblock from higorderatomic to
> something. In that case, highatomic pageblock accounting is broken
> so it doesn't work(e.g., VM cannot reserve highorderatomic pageblocks
> any more although it doesn't reach 1% limit).
>
> So, this patch prohibits the changing from highatomic to other type.
> It's no problem because MIGRATE_HIGHATOMIC is not listed in fallback
> array so stealing will only happen due to unexpected races which is
> really rare. Also, such prohibiting keeps highatomic pageblock more
> longer so it would be better for highorderatomic page allocation.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/page_alloc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 55ad0229ebf3..79853b258211 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2154,7 +2154,8 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
>
>  		page = list_first_entry(&area->free_list[fallback_mt],
>  						struct page, lru);
> -		if (can_steal)
> +		if (can_steal &&
> +			get_pageblock_migratetype(page) != MIGRATE_HIGHATOMIC)
>  			steal_suitable_fallback(zone, page, start_migratetype);
>
>  		/* Remove the page from the freelists */
> @@ -2555,7 +2556,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
>  		struct page *endpage = page + (1 << order) - 1;
>  		for (; page < endpage; page += pageblock_nr_pages) {
>  			int mt = get_pageblock_migratetype(page);
> -			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt))
> +			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)
> +				&& mt != MIGRATE_HIGHATOMIC)
>  				set_pageblock_migratetype(page,
>  							  MIGRATE_MOVABLE);
>  		}
>

[toc] | [prev] | [next] | [standalone]

#1499349 — Re: [PATCH 1/4] mm: adjust reserved highatomic count

On Tue, Oct 11, 2016 at 01:19:16PM +0900, Minchan Kim wrote:
> From 4a0b6a74ebf1af7f90720b0028da49e2e2a2b679 Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Thu, 6 Oct 2016 13:38:35 +0900
> Subject: [PATCH] mm: don't steal highatomic pageblock
> 
> In page freeing path, migratetype is racy so that a highorderatomic
> page could free into non-highorderatomic free list. If that page
> is allocated, VM can change the pageblock from higorderatomic to
> something. In that case, highatomic pageblock accounting is broken
> so it doesn't work(e.g., VM cannot reserve highorderatomic pageblocks
> any more although it doesn't reach 1% limit).
> 
> So, this patch prohibits the changing from highatomic to other type.
> It's no problem because MIGRATE_HIGHATOMIC is not listed in fallback
> array so stealing will only happen due to unexpected races which is
> really rare. Also, such prohibiting keeps highatomic pageblock more
> longer so it would be better for highorderatomic page allocation.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/page_alloc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 55ad0229ebf3..79853b258211 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2154,7 +2154,8 @@ __rmqueue_fallback(struct zone *zone, unsigned int order, int start_migratetype)
>  
>  		page = list_first_entry(&area->free_list[fallback_mt],
>  						struct page, lru);
> -		if (can_steal)
> +		if (can_steal &&
> +			get_pageblock_migratetype(page) != MIGRATE_HIGHATOMIC)
>  			steal_suitable_fallback(zone, page, start_migratetype);
>  
>  		/* Remove the page from the freelists */
> @@ -2555,7 +2556,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
>  		struct page *endpage = page + (1 << order) - 1;
>  		for (; page < endpage; page += pageblock_nr_pages) {
>  			int mt = get_pageblock_migratetype(page);
> -			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt))
> +			if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)
> +				&& mt != MIGRATE_HIGHATOMIC)
>  				set_pageblock_migratetype(page,
>  							  MIGRATE_MOVABLE);
>  		}

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1497024 — [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

After fixing the race of highatomic page count, I still encounter
OOM with many free memory reserved as highatomic.

One of reason in my testing was we unreserve free pages only if
reclaim has progress. Otherwise, we cannot have chance to unreseve.

Other problem after fixing it was it doesn't guarantee every pages
unreserving of highatomic pageblock because it just release *a*
pageblock which could have few free pages so other context could
steal it easily so that the process stucked with direct reclaim
finally can encounter OOM although there are free pages which can
be unreserved.

This patch changes the logic so that it unreserves pageblocks with
no_progress_loop proportionally. IOW, in first retrial of reclaim,
it will try to unreserve a pageblock. In second retrial of reclaim,
it will try to unreserve 1/MAX_RECLAIM_RETRIES * reserved_pageblock
and finally all reserved pageblock before the OOM.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/page_alloc.c | 57 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d110cd640264..eeb047bb0e9d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -71,6 +71,12 @@
 #include <asm/div64.h>
 #include "internal.h"
 
+/*
+ * Maximum number of reclaim retries without any progress before OOM killer
+ * is consider as the only way to move forward.
+ */
+#define MAX_RECLAIM_RETRIES 16
+
 /* prevent >1 _updater_ of zone percpu pageset ->high and ->batch fields */
 static DEFINE_MUTEX(pcp_batch_high_lock);
 #define MIN_PERCPU_PAGELIST_FRACTION	(8)
@@ -2107,7 +2113,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
  * intense memory pressure but failed atomic allocations should be easier
  * to recover from than an OOM.
  */
-static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
+static int unreserve_highatomic_pageblock(const struct alloc_context *ac,
+						int no_progress_loops)
 {
 	struct zonelist *zonelist = ac->zonelist;
 	unsigned long flags;
@@ -2115,15 +2122,40 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 	struct zone *zone;
 	struct page *page;
 	int order;
+	int unreserved_pages = 0;
 
 	for_each_zone_zonelist_nodemask(zone, z, zonelist, ac->high_zoneidx,
 								ac->nodemask) {
-		/* Preserve at least one pageblock */
-		if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
+		unsigned long unreserve_pages_max;
+
+		/*
+		 * Try to preserve at least one pageblock but use up before
+		 * OOM kill.
+		 */
+		if (no_progress_loops < MAX_RECLAIM_RETRIES &&
+			zone->nr_reserved_highatomic <= pageblock_nr_pages)
 			continue;
 
 		spin_lock_irqsave(&zone->lock, flags);
-		for (order = 0; order < MAX_ORDER; order++) {
+		if (no_progress_loops < MAX_RECLAIM_RETRIES) {
+			unreserve_pages_max = no_progress_loops *
+					zone->nr_reserved_highatomic /
+					MAX_RECLAIM_RETRIES;
+			unreserve_pages_max = max(unreserve_pages_max,
+						pageblock_nr_pages);
+		} else {
+			/*
+			 * By race with page free functions, !highatomic
+			 * pageblocks can have a free page in highatomic
+			 * migratetype free list. So if we are about to
+			 * kill some process, unreserve every free pages
+			 * in highorderatomic.
+			 */
+			unreserve_pages_max = -1UL;
+		}
+
+		for (order = 0; order < MAX_ORDER &&
+				unreserve_pages_max > 0; order++) {
 			struct free_area *area = &(zone->free_area[order]);
 
 			page = list_first_entry_or_null(
@@ -2151,6 +2183,9 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 				zone->nr_reserved_highatomic -= min(
 						pageblock_nr_pages,
 						zone->nr_reserved_highatomic);
+				unreserve_pages_max -= min(pageblock_nr_pages,
+					zone->nr_reserved_highatomic);
+				unreserved_pages += 1 << page_order(page);
 			}
 
 			/*
@@ -2164,11 +2199,11 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 			 */
 			set_pageblock_migratetype(page, ac->migratetype);
 			move_freepages_block(zone, page, ac->migratetype);
-			spin_unlock_irqrestore(&zone->lock, flags);
-			return;
 		}
 		spin_unlock_irqrestore(&zone->lock, flags);
 	}
+
+	return unreserved_pages;
 }
 
 /* Remove an element from the buddy allocator from the fallback list */
@@ -3370,7 +3405,6 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
 	 * Shrink them them and try again
 	 */
 	if (!page && !drained) {
-		unreserve_highatomic_pageblock(ac);
 		drain_all_pages(NULL);
 		drained = true;
 		goto retry;
@@ -3449,12 +3483,6 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 }
 
 /*
- * Maximum number of reclaim retries without any progress before OOM killer
- * is consider as the only way to move forward.
- */
-#define MAX_RECLAIM_RETRIES 16
-
-/*
  * Checks whether it makes sense to retry the reclaim to make a forward progress
  * for the given allocation request.
  * The reclaim feedback represented by did_some_progress (any progress during
@@ -3490,6 +3518,9 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
 		return false;
 
+	if (unreserve_highatomic_pageblock(ac, *no_progress_loops))
+		return true;
+
 	/*
 	 * Keep reclaiming pages while there is a chance this will lead
 	 * somewhere.  If none of the target zones can satisfy our allocation
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1497094 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Fri 07-10-16 14:45:35, Minchan Kim wrote:
> After fixing the race of highatomic page count, I still encounter
> OOM with many free memory reserved as highatomic.
> 
> One of reason in my testing was we unreserve free pages only if
> reclaim has progress. Otherwise, we cannot have chance to unreseve.
> 
> Other problem after fixing it was it doesn't guarantee every pages
> unreserving of highatomic pageblock because it just release *a*
> pageblock which could have few free pages so other context could
> steal it easily so that the process stucked with direct reclaim
> finally can encounter OOM although there are free pages which can
> be unreserved.
> 
> This patch changes the logic so that it unreserves pageblocks with
> no_progress_loop proportionally. IOW, in first retrial of reclaim,
> it will try to unreserve a pageblock. In second retrial of reclaim,
> it will try to unreserve 1/MAX_RECLAIM_RETRIES * reserved_pageblock
> and finally all reserved pageblock before the OOM.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/page_alloc.c | 57 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 13 deletions(-)

This sounds much more complex then it needs to be IMHO. Why something as
simple as thhe following wouldn't work? Please note that I even didn't
try to compile this. It is just give you an idea.
---
 mm/page_alloc.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 73f60ad6315f..e575a4f38555 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2056,7 +2056,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
  * intense memory pressure but failed atomic allocations should be easier
  * to recover from than an OOM.
  */
-static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
+static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
+		bool force)
 {
 	struct zonelist *zonelist = ac->zonelist;
 	unsigned long flags;
@@ -2067,8 +2068,14 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 
 	for_each_zone_zonelist_nodemask(zone, z, zonelist, ac->high_zoneidx,
 								ac->nodemask) {
-		/* Preserve at least one pageblock */
-		if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
+		if (!zone->nr_reserved_highatomic)
+			continue;
+
+		/*
+		 * Preserve at least one pageblock unless we are really running
+		 * out of memory
+		 */
+		if (!force && zone->nr_reserved_highatomic <= pageblock_nr_pages)
 			continue;
 
 		spin_lock_irqsave(&zone->lock, flags);
@@ -2102,10 +2109,12 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 			set_pageblock_migratetype(page, ac->migratetype);
 			move_freepages_block(zone, page, ac->migratetype);
 			spin_unlock_irqrestore(&zone->lock, flags);
-			return;
+			return true;
 		}
 		spin_unlock_irqrestore(&zone->lock, flags);
 	}
+
+	return false;
 }
 
 /* Remove an element from the buddy allocator from the fallback list */
@@ -3302,7 +3311,7 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
 	 * Shrink them them and try again
 	 */
 	if (!page && !drained) {
-		unreserve_highatomic_pageblock(ac);
+		unreserve_highatomic_pageblock(ac, false);
 		drain_all_pages(NULL);
 		drained = true;
 		goto retry;
@@ -3418,9 +3427,14 @@ 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.
+	 * Do last desparate attempt to throw high atomic reserves away
+	 * before we give up
 	 */
-	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
+	if (*no_progress_loops > MAX_RECLAIM_RETRIES) {
+		if (unreserve_highatomic_pageblock(ac, true))
+			return true;
 		return false;
+	}
 
 	/*
 	 * Keep reclaiming pages while there is a chance this will lead
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1497197 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Fri, Oct 07, 2016 at 11:09:17AM +0200, Michal Hocko wrote:
> On Fri 07-10-16 14:45:35, Minchan Kim wrote:
> > After fixing the race of highatomic page count, I still encounter
> > OOM with many free memory reserved as highatomic.
> > 
> > One of reason in my testing was we unreserve free pages only if
> > reclaim has progress. Otherwise, we cannot have chance to unreseve.
> > 
> > Other problem after fixing it was it doesn't guarantee every pages
> > unreserving of highatomic pageblock because it just release *a*
> > pageblock which could have few free pages so other context could
> > steal it easily so that the process stucked with direct reclaim
> > finally can encounter OOM although there are free pages which can
> > be unreserved.
> > 
> > This patch changes the logic so that it unreserves pageblocks with
> > no_progress_loop proportionally. IOW, in first retrial of reclaim,
> > it will try to unreserve a pageblock. In second retrial of reclaim,
> > it will try to unreserve 1/MAX_RECLAIM_RETRIES * reserved_pageblock
> > and finally all reserved pageblock before the OOM.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/page_alloc.c | 57 ++++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 44 insertions(+), 13 deletions(-)
> 
> This sounds much more complex then it needs to be IMHO. Why something as
> simple as thhe following wouldn't work? Please note that I even didn't
> try to compile this. It is just give you an idea.
> ---
>  mm/page_alloc.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 73f60ad6315f..e575a4f38555 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2056,7 +2056,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
>   * intense memory pressure but failed atomic allocations should be easier
>   * to recover from than an OOM.
>   */
> -static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> +static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
> +		bool force)
>  {
>  	struct zonelist *zonelist = ac->zonelist;
>  	unsigned long flags;
> @@ -2067,8 +2068,14 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
>  
>  	for_each_zone_zonelist_nodemask(zone, z, zonelist, ac->high_zoneidx,
>  								ac->nodemask) {
> -		/* Preserve at least one pageblock */
> -		if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
> +		if (!zone->nr_reserved_highatomic)
> +			continue;
> +
> +		/*
> +		 * Preserve at least one pageblock unless we are really running
> +		 * out of memory
> +		 */
> +		if (!force && zone->nr_reserved_highatomic <= pageblock_nr_pages)
>  			continue;
>  
>  		spin_lock_irqsave(&zone->lock, flags);
> @@ -2102,10 +2109,12 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
>  			set_pageblock_migratetype(page, ac->migratetype);
>  			move_freepages_block(zone, page, ac->migratetype);
>  			spin_unlock_irqrestore(&zone->lock, flags);
> -			return;
> +			return true;

Such cut-off makes reserved pageblock remained before the OOM.
We call it as premature OOM kill.

If you feel it's rather complex, simply, we can drain *every*
reserved pages when no_progress_loops is greater than MAX_RECLAIM_RETRIES.
Do you want it?

It's rather conservative approach to keep highatomic pageblocks.
Anyway, I think it's matter of policy and my goal is just to use up
every reserved pageblock before OOM so anyone never say
"Hey, enough free pages which is greater than min watermark but
why I should see the OOM with GFP_KERNEL 4K allocation".

so I'm not against on it if you guys like it. Say your preference.


>  		}
>  		spin_unlock_irqrestore(&zone->lock, flags);
>  	}
> +
> +	return false;
>  }
>  
>  /* Remove an element from the buddy allocator from the fallback list */
> @@ -3302,7 +3311,7 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
>  	 * Shrink them them and try again
>  	 */
>  	if (!page && !drained) {
> -		unreserve_highatomic_pageblock(ac);
> +		unreserve_highatomic_pageblock(ac, false);
>  		drain_all_pages(NULL);
>  		drained = true;
>  		goto retry;
> @@ -3418,9 +3427,14 @@ 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.
> +	 * Do last desparate attempt to throw high atomic reserves away
> +	 * before we give up
>  	 */
> -	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES) {
> +		if (unreserve_highatomic_pageblock(ac, true))
> +			return true;
>  		return false;
> +	}
>  
>  	/*
>  	 * Keep reclaiming pages while there is a chance this will lead
> -- 
> Michal Hocko
> SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1498153 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Fri 07-10-16 23:43:45, Minchan Kim wrote:
> On Fri, Oct 07, 2016 at 11:09:17AM +0200, Michal Hocko wrote:
> > On Fri 07-10-16 14:45:35, Minchan Kim wrote:
> > > After fixing the race of highatomic page count, I still encounter
> > > OOM with many free memory reserved as highatomic.
> > > 
> > > One of reason in my testing was we unreserve free pages only if
> > > reclaim has progress. Otherwise, we cannot have chance to unreseve.
> > > 
> > > Other problem after fixing it was it doesn't guarantee every pages
> > > unreserving of highatomic pageblock because it just release *a*
> > > pageblock which could have few free pages so other context could
> > > steal it easily so that the process stucked with direct reclaim
> > > finally can encounter OOM although there are free pages which can
> > > be unreserved.
> > > 
> > > This patch changes the logic so that it unreserves pageblocks with
> > > no_progress_loop proportionally. IOW, in first retrial of reclaim,
> > > it will try to unreserve a pageblock. In second retrial of reclaim,
> > > it will try to unreserve 1/MAX_RECLAIM_RETRIES * reserved_pageblock
> > > and finally all reserved pageblock before the OOM.
> > > 
> > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > > ---
> > >  mm/page_alloc.c | 57 ++++++++++++++++++++++++++++++++++++++++++++-------------
> > >  1 file changed, 44 insertions(+), 13 deletions(-)
> > 
> > This sounds much more complex then it needs to be IMHO. Why something as
> > simple as thhe following wouldn't work? Please note that I even didn't
> > try to compile this. It is just give you an idea.
> > ---
> >  mm/page_alloc.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 73f60ad6315f..e575a4f38555 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2056,7 +2056,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
> >   * intense memory pressure but failed atomic allocations should be easier
> >   * to recover from than an OOM.
> >   */
> > -static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > +static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
> > +		bool force)
> >  {
> >  	struct zonelist *zonelist = ac->zonelist;
> >  	unsigned long flags;
> > @@ -2067,8 +2068,14 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> >  
> >  	for_each_zone_zonelist_nodemask(zone, z, zonelist, ac->high_zoneidx,
> >  								ac->nodemask) {
> > -		/* Preserve at least one pageblock */
> > -		if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
> > +		if (!zone->nr_reserved_highatomic)
> > +			continue;
> > +
> > +		/*
> > +		 * Preserve at least one pageblock unless we are really running
> > +		 * out of memory
> > +		 */
> > +		if (!force && zone->nr_reserved_highatomic <= pageblock_nr_pages)
> >  			continue;
> >  
> >  		spin_lock_irqsave(&zone->lock, flags);
> > @@ -2102,10 +2109,12 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> >  			set_pageblock_migratetype(page, ac->migratetype);
> >  			move_freepages_block(zone, page, ac->migratetype);
> >  			spin_unlock_irqrestore(&zone->lock, flags);
> > -			return;
> > +			return true;
> 
> Such cut-off makes reserved pageblock remained before the OOM.
> We call it as premature OOM kill.

Not sure I understand. The above should get rid of all atomic reserves
before we go OOM. We can do it all at once but that sounds too
aggressive to me. If we just do one at the time we have a chance to
keep some reserves if the OOM situation is really ephemeral.

Does this patch work in your usecase?

> If you feel it's rather complex, simply, we can drain *every*
> reserved pages when no_progress_loops is greater than MAX_RECLAIM_RETRIES.
> Do you want it?
> 
> It's rather conservative approach to keep highatomic pageblocks.
> Anyway, I think it's matter of policy and my goal is just to use up
> every reserved pageblock before OOM so anyone never say
> "Hey, enough free pages which is greater than min watermark but
> why I should see the OOM with GFP_KERNEL 4K allocation".

I agree that triggering OOM while we are above min watermarks is less
than optimial and we should think about how to fix it.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1498607 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

Hi Michal,

On Mon, Oct 10, 2016 at 09:41:40AM +0200, Michal Hocko wrote:
> On Fri 07-10-16 23:43:45, Minchan Kim wrote:
> > On Fri, Oct 07, 2016 at 11:09:17AM +0200, Michal Hocko wrote:
> > > On Fri 07-10-16 14:45:35, Minchan Kim wrote:
> > > > After fixing the race of highatomic page count, I still encounter
> > > > OOM with many free memory reserved as highatomic.
> > > > 
> > > > One of reason in my testing was we unreserve free pages only if
> > > > reclaim has progress. Otherwise, we cannot have chance to unreseve.
> > > > 
> > > > Other problem after fixing it was it doesn't guarantee every pages
> > > > unreserving of highatomic pageblock because it just release *a*
> > > > pageblock which could have few free pages so other context could
> > > > steal it easily so that the process stucked with direct reclaim
> > > > finally can encounter OOM although there are free pages which can
> > > > be unreserved.
> > > > 
> > > > This patch changes the logic so that it unreserves pageblocks with
> > > > no_progress_loop proportionally. IOW, in first retrial of reclaim,
> > > > it will try to unreserve a pageblock. In second retrial of reclaim,
> > > > it will try to unreserve 1/MAX_RECLAIM_RETRIES * reserved_pageblock
> > > > and finally all reserved pageblock before the OOM.
> > > > 
> > > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > > > ---
> > > >  mm/page_alloc.c | 57 ++++++++++++++++++++++++++++++++++++++++++++-------------
> > > >  1 file changed, 44 insertions(+), 13 deletions(-)
> > > 
> > > This sounds much more complex then it needs to be IMHO. Why something as
> > > simple as thhe following wouldn't work? Please note that I even didn't
> > > try to compile this. It is just give you an idea.
> > > ---
> > >  mm/page_alloc.c | 26 ++++++++++++++++++++------
> > >  1 file changed, 20 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 73f60ad6315f..e575a4f38555 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -2056,7 +2056,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
> > >   * intense memory pressure but failed atomic allocations should be easier
> > >   * to recover from than an OOM.
> > >   */
> > > -static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > > +static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
> > > +		bool force)
> > >  {
> > >  	struct zonelist *zonelist = ac->zonelist;
> > >  	unsigned long flags;
> > > @@ -2067,8 +2068,14 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > >  
> > >  	for_each_zone_zonelist_nodemask(zone, z, zonelist, ac->high_zoneidx,
> > >  								ac->nodemask) {
> > > -		/* Preserve at least one pageblock */
> > > -		if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
> > > +		if (!zone->nr_reserved_highatomic)
> > > +			continue;
> > > +
> > > +		/*
> > > +		 * Preserve at least one pageblock unless we are really running
> > > +		 * out of memory
> > > +		 */
> > > +		if (!force && zone->nr_reserved_highatomic <= pageblock_nr_pages)
> > >  			continue;
> > >  
> > >  		spin_lock_irqsave(&zone->lock, flags);
> > > @@ -2102,10 +2109,12 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > >  			set_pageblock_migratetype(page, ac->migratetype);
> > >  			move_freepages_block(zone, page, ac->migratetype);
> > >  			spin_unlock_irqrestore(&zone->lock, flags);
> > > -			return;
> > > +			return true;
> > 
> > Such cut-off makes reserved pageblock remained before the OOM.
> > We call it as premature OOM kill.
> 
> Not sure I understand. The above should get rid of all atomic reserves
> before we go OOM. We can do it all at once but that sounds too

The problem is there is race between page freeing path and unreserve
logic so that some pages could be in highatomic free list even though
zone->nr_reserved_highatomic is already zero.
So, at least, it would be better to have a draining step at some point
where was (no_progress_loops == MAX_RECLAIM RETRIES) in my patch.

Also, your patch makes retry loop greater than MAX_RECLAIM_RETRIES
if unreserve_highatomic_pageblock returns true. Theoretically,
it would make live lock. You might argue it's *really really* rare
but I don't want to add such subtle thing.
Maybe, we could drain when no_progress_loops == MAX_RECLAIM_RETRIES.

> aggressive to me. If we just do one at the time we have a chance to
> keep some reserves if the OOM situation is really ephemeral.
> 
> Does this patch work in your usecase?

I didn't test but I guess it works but it has problems I mentioned
above. 

[toc] | [prev] | [next] | [standalone]

#1498629 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Tue 11-10-16 14:01:41, Minchan Kim wrote:
> Hi Michal,
> 
> On Mon, Oct 10, 2016 at 09:41:40AM +0200, Michal Hocko wrote:
> > On Fri 07-10-16 23:43:45, Minchan Kim wrote:
> > > On Fri, Oct 07, 2016 at 11:09:17AM +0200, Michal Hocko wrote:
[...]
> > > > @@ -2102,10 +2109,12 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > > >  			set_pageblock_migratetype(page, ac->migratetype);
> > > >  			move_freepages_block(zone, page, ac->migratetype);
> > > >  			spin_unlock_irqrestore(&zone->lock, flags);
> > > > -			return;
> > > > +			return true;
> > > 
> > > Such cut-off makes reserved pageblock remained before the OOM.
> > > We call it as premature OOM kill.
> > 
> > Not sure I understand. The above should get rid of all atomic reserves
> > before we go OOM. We can do it all at once but that sounds too
> 
> The problem is there is race between page freeing path and unreserve
> logic so that some pages could be in highatomic free list even though
> zone->nr_reserved_highatomic is already zero.

Does it make any sense to handle such an unlikely case?

> So, at least, it would be better to have a draining step at some point
> where was (no_progress_loops == MAX_RECLAIM RETRIES) in my patch.
> 
> Also, your patch makes retry loop greater than MAX_RECLAIM_RETRIES
> if unreserve_highatomic_pageblock returns true. Theoretically,
> it would make live lock. You might argue it's *really really* rare
> but I don't want to add such subtle thing.
> Maybe, we could drain when no_progress_loops == MAX_RECLAIM_RETRIES.

What would be the scenario when we would really livelock here? How can
we have unreserve_highatomic_pageblock returning true for ever?

> > aggressive to me. If we just do one at the time we have a chance to
> > keep some reserves if the OOM situation is really ephemeral.
> > 
> > Does this patch work in your usecase?
> 
> I didn't test but I guess it works but it has problems I mentioned
> above. 

Please do not make this too over complicated and be practical. I do not
really want to dismiss your usecase but I am really not convinced that
such a "perfectly fit into all memory" situations are sustainable and
justify to make the whole code more complex. I agree that we can at
least try to do something to release those reserves but let's do it
as simple as possible.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1498632 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Tue, Oct 11, 2016 at 08:50:48AM +0200, Michal Hocko wrote:
> On Tue 11-10-16 14:01:41, Minchan Kim wrote:
> > Hi Michal,
> > 
> > On Mon, Oct 10, 2016 at 09:41:40AM +0200, Michal Hocko wrote:
> > > On Fri 07-10-16 23:43:45, Minchan Kim wrote:
> > > > On Fri, Oct 07, 2016 at 11:09:17AM +0200, Michal Hocko wrote:
> [...]
> > > > > @@ -2102,10 +2109,12 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > > > >  			set_pageblock_migratetype(page, ac->migratetype);
> > > > >  			move_freepages_block(zone, page, ac->migratetype);
> > > > >  			spin_unlock_irqrestore(&zone->lock, flags);
> > > > > -			return;
> > > > > +			return true;
> > > > 
> > > > Such cut-off makes reserved pageblock remained before the OOM.
> > > > We call it as premature OOM kill.
> > > 
> > > Not sure I understand. The above should get rid of all atomic reserves
> > > before we go OOM. We can do it all at once but that sounds too
> > 
> > The problem is there is race between page freeing path and unreserve
> > logic so that some pages could be in highatomic free list even though
> > zone->nr_reserved_highatomic is already zero.
> 
> Does it make any sense to handle such an unlikely case?

I agree if it's really hard to solve but why should we remain
such hole in the algorithm if we can fix easily?

> 
> > So, at least, it would be better to have a draining step at some point
> > where was (no_progress_loops == MAX_RECLAIM RETRIES) in my patch.
> > 
> > Also, your patch makes retry loop greater than MAX_RECLAIM_RETRIES
> > if unreserve_highatomic_pageblock returns true. Theoretically,
> > it would make live lock. You might argue it's *really really* rare
> > but I don't want to add such subtle thing.
> > Maybe, we could drain when no_progress_loops == MAX_RECLAIM_RETRIES.
> 
> What would be the scenario when we would really livelock here? How can
> we have unreserve_highatomic_pageblock returning true for ever?

Other context freeing highorder page/reallocating repeatedly while
a process stucked direct reclaim is looping with should_reclaim_retry.

> 
> > > aggressive to me. If we just do one at the time we have a chance to
> > > keep some reserves if the OOM situation is really ephemeral.
> > > 
> > > Does this patch work in your usecase?
> > 
> > I didn't test but I guess it works but it has problems I mentioned
> > above. 
> 
> Please do not make this too over complicated and be practical. I do not
> really want to dismiss your usecase but I am really not convinced that
> such a "perfectly fit into all memory" situations are sustainable and
> justify to make the whole code more complex. I agree that we can at
> least try to do something to release those reserves but let's do it
> as simple as possible.

If you think it's too complicated, how about this?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index fd91b8955b26..e3ce442e9976 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2098,7 +2098,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
  * intense memory pressure but failed atomic allocations should be easier
  * to recover from than an OOM.
  */
-static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
+static bool unreserve_highatomic_pageblock(const struct alloc_context *ac,
+						bool drain)
 {
 	struct zonelist *zonelist = ac->zonelist;
 	unsigned long flags;
@@ -2106,11 +2107,12 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 	struct zone *zone;
 	struct page *page;
 	int order;
+	bool ret = false;
 
 	for_each_zone_zonelist_nodemask(zone, z, zonelist, ac->high_zoneidx,
 								ac->nodemask) {
 		/* Preserve at least one pageblock */
-		if (zone->nr_reserved_highatomic <= pageblock_nr_pages)
+		if (!drain && zone->nr_reserved_highatomic <= pageblock_nr_pages)
 			continue;
 
 		spin_lock_irqsave(&zone->lock, flags);
@@ -2154,12 +2156,24 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
 			 * may increase.
 			 */
 			set_pageblock_migratetype(page, ac->migratetype);
-			move_freepages_block(zone, page, ac->migratetype);
-			spin_unlock_irqrestore(&zone->lock, flags);
-			return;
+			ret = move_freepages_block(zone, page,
+						ac->migratetype);
+			/*
+			 * By race with page freeing functions, !highatomic
+			 * pageblocks can have free pages in highatomic free
+			 * list so if drain is true, try to unreserve every
+			 * free pages in highatomic free list without bailing
+			 * out.
+			 */
+			if (!drain) {
+				spin_unlock_irqrestore(&zone->lock, flags);
+				return ret;
+			}
 		}
 		spin_unlock_irqrestore(&zone->lock, flags);
 	}
+
+	return ret;
 }
 
 /* Remove an element from the buddy allocator from the fallback list */
@@ -3358,7 +3372,7 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
 	 * Shrink them them and try again
 	 */
 	if (!page && !drained) {
-		unreserve_highatomic_pageblock(ac);
+		unreserve_highatomic_pageblock(ac, false);
 		drain_all_pages(NULL);
 		drained = true;
 		goto retry;
@@ -3475,8 +3489,11 @@ 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)
+	if (*no_progress_loops > MAX_RECLAIM_RETRIES) {
+		if (unreserve_highatomic_pageblock(ac, true))
+			return true;
 		return false;
+	}
 
 	/*
 	 * Keep reclaiming pages while there is a chance this will lead
> 
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[toc] | [prev] | [next] | [standalone]

#1498635 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Tue 11-10-16 16:09:45, Minchan Kim wrote:
> On Tue, Oct 11, 2016 at 08:50:48AM +0200, Michal Hocko wrote:
> > On Tue 11-10-16 14:01:41, Minchan Kim wrote:
[...]
> > > Also, your patch makes retry loop greater than MAX_RECLAIM_RETRIES
> > > if unreserve_highatomic_pageblock returns true. Theoretically,
> > > it would make live lock. You might argue it's *really really* rare
> > > but I don't want to add such subtle thing.
> > > Maybe, we could drain when no_progress_loops == MAX_RECLAIM_RETRIES.
> > 
> > What would be the scenario when we would really livelock here? How can
> > we have unreserve_highatomic_pageblock returning true for ever?
> 
> Other context freeing highorder page/reallocating repeatedly while
> a process stucked direct reclaim is looping with should_reclaim_retry.

If we unreserve those pages then we should converge to OOM. Btw. this
can happen even without highmem reserves. Heavy short lived allocations
might keep us looping at the lowest priority. They are just too unlikely
to care about.

> > > > aggressive to me. If we just do one at the time we have a chance to
> > > > keep some reserves if the OOM situation is really ephemeral.
> > > > 
> > > > Does this patch work in your usecase?
> > > 
> > > I didn't test but I guess it works but it has problems I mentioned
> > > above. 
> > 
> > Please do not make this too over complicated and be practical. I do not
> > really want to dismiss your usecase but I am really not convinced that
> > such a "perfectly fit into all memory" situations are sustainable and
> > justify to make the whole code more complex. I agree that we can at
> > least try to do something to release those reserves but let's do it
> > as simple as possible.
> 
> If you think it's too complicated, how about this?

Definitely better than the original patch. Little bit too aggressive
because we could really go with one block at the time. But this is a
minor thing and easily fixable...

> @@ -2154,12 +2156,24 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
>  			 * may increase.
>  			 */
>  			set_pageblock_migratetype(page, ac->migratetype);
> -			move_freepages_block(zone, page, ac->migratetype);
> -			spin_unlock_irqrestore(&zone->lock, flags);
> -			return;
> +			ret = move_freepages_block(zone, page,
> +						ac->migratetype);
> +			/*
> +			 * By race with page freeing functions, !highatomic
> +			 * pageblocks can have free pages in highatomic free
> +			 * list so if drain is true, try to unreserve every
> +			 * free pages in highatomic free list without bailing
> +			 * out.
> +			 */
> +			if (!drain) {

			if (ret)
> +				spin_unlock_irqrestore(&zone->lock, flags);
> +				return ret;
> +			}

arguably this would work better also for !drain case which currently
tries to unreserve but in case of the race it would do nothing.

>  		}
>  		spin_unlock_irqrestore(&zone->lock, flags);
>  	}
> +
> +	return ret;
>  }
>  
>  /* Remove an element from the buddy allocator from the fallback list */
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1498636 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Tue, Oct 11, 2016 at 09:26:06AM +0200, Michal Hocko wrote:
> On Tue 11-10-16 16:09:45, Minchan Kim wrote:
> > On Tue, Oct 11, 2016 at 08:50:48AM +0200, Michal Hocko wrote:
> > > On Tue 11-10-16 14:01:41, Minchan Kim wrote:
> [...]
> > > > Also, your patch makes retry loop greater than MAX_RECLAIM_RETRIES
> > > > if unreserve_highatomic_pageblock returns true. Theoretically,
> > > > it would make live lock. You might argue it's *really really* rare
> > > > but I don't want to add such subtle thing.
> > > > Maybe, we could drain when no_progress_loops == MAX_RECLAIM_RETRIES.
> > > 
> > > What would be the scenario when we would really livelock here? How can
> > > we have unreserve_highatomic_pageblock returning true for ever?
> > 
> > Other context freeing highorder page/reallocating repeatedly while
> > a process stucked direct reclaim is looping with should_reclaim_retry.
> 
> If we unreserve those pages then we should converge to OOM. Btw. this
> can happen even without highmem reserves. Heavy short lived allocations
> might keep us looping at the lowest priority. They are just too unlikely
> to care about.

Indeed.
> 
> > > > > aggressive to me. If we just do one at the time we have a chance to
> > > > > keep some reserves if the OOM situation is really ephemeral.
> > > > > 
> > > > > Does this patch work in your usecase?
> > > > 
> > > > I didn't test but I guess it works but it has problems I mentioned
> > > > above. 
> > > 
> > > Please do not make this too over complicated and be practical. I do not
> > > really want to dismiss your usecase but I am really not convinced that
> > > such a "perfectly fit into all memory" situations are sustainable and
> > > justify to make the whole code more complex. I agree that we can at
> > > least try to do something to release those reserves but let's do it
> > > as simple as possible.
> > 
> > If you think it's too complicated, how about this?
> 
> Definitely better than the original patch. Little bit too aggressive
> because we could really go with one block at the time. But this is a
> minor thing and easily fixable...
> 
> > @@ -2154,12 +2156,24 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> >  			 * may increase.
> >  			 */
> >  			set_pageblock_migratetype(page, ac->migratetype);
> > -			move_freepages_block(zone, page, ac->migratetype);
> > -			spin_unlock_irqrestore(&zone->lock, flags);
> > -			return;
> > +			ret = move_freepages_block(zone, page,
> > +						ac->migratetype);
> > +			/*
> > +			 * By race with page freeing functions, !highatomic
> > +			 * pageblocks can have free pages in highatomic free
> > +			 * list so if drain is true, try to unreserve every
> > +			 * free pages in highatomic free list without bailing
> > +			 * out.
> > +			 */
> > +			if (!drain) {
> 
> 			if (ret)
> > +				spin_unlock_irqrestore(&zone->lock, flags);
> > +				return ret;
> > +			}
> 
> arguably this would work better also for !drain case which currently
> tries to unreserve but in case of the race it would do nothing.

I thought it but I was afraid if you say again it's over complicated.
I will do it with your SOB in next spin.

Thanks, Michal.

> 
> >  		}
> >  		spin_unlock_irqrestore(&zone->lock, flags);
> >  	}
> > +
> > +	return ret;
> >  }
> >  
> >  /* Remove an element from the buddy allocator from the fallback list */
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[toc] | [prev] | [next] | [standalone]

#1498652 — Re: [PATCH 3/4] mm: unreserve highatomic free pages fully before OOM

On Tue 11-10-16 16:37:16, Minchan Kim wrote:
> On Tue, Oct 11, 2016 at 09:26:06AM +0200, Michal Hocko wrote:
> > On Tue 11-10-16 16:09:45, Minchan Kim wrote:
[...]
> > > @@ -2154,12 +2156,24 @@ static void unreserve_highatomic_pageblock(const struct alloc_context *ac)
> > >  			 * may increase.
> > >  			 */
> > >  			set_pageblock_migratetype(page, ac->migratetype);
> > > -			move_freepages_block(zone, page, ac->migratetype);
> > > -			spin_unlock_irqrestore(&zone->lock, flags);
> > > -			return;
> > > +			ret = move_freepages_block(zone, page,
> > > +						ac->migratetype);
> > > +			/*
> > > +			 * By race with page freeing functions, !highatomic
> > > +			 * pageblocks can have free pages in highatomic free
> > > +			 * list so if drain is true, try to unreserve every
> > > +			 * free pages in highatomic free list without bailing
> > > +			 * out.
> > > +			 */
> > > +			if (!drain) {
> > 
> > 			if (ret)
> > > +				spin_unlock_irqrestore(&zone->lock, flags);
> > > +				return ret;
> > > +			}
> > 
> > arguably this would work better also for !drain case which currently
> > tries to unreserve but in case of the race it would do nothing.
> 
> I thought it but I was afraid if you say again it's over complicated.

Well, maybe there is even better/easier solution. Anyway, if
I were you I would just split it into two patches. The first
to unreserve from shoudl_reclaim_retry and the later to make
unreserve_highatomic_pageblock more reliable.

> I will do it with your SOB in next spin.

ok, thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1497097

On Fri 07-10-16 14:45:32, Minchan Kim wrote:
> I got OOM report from production team with v4.4 kernel.
> It has enough free memory but failed to allocate order-0 page and
> finally encounter OOM kill.
> I could reproduce it with my test easily. Look at below.
> The reason is free pages(19M) of DMA32 zone are reserved for
> HIGHORDERATOMIC and doesn't unreserved before the OOM.

Is this really reproducible?

[...]
> active_anon:383949 inactive_anon:106724 isolated_anon:0
>  active_file:15 inactive_file:44 isolated_file:0
>  unevictable:0 dirty:0 writeback:24 unstable:0
>  slab_reclaimable:2483 slab_unreclaimable:3326
>  mapped:0 shmem:0 pagetables:1906 bounce:0
>  free:6898 free_pcp:291 free_cma:0
[...]
> Free swap  = 8kB
> Total swap = 255996kB
> 524158 pages RAM
> 0 pages HighMem/MovableOnly
> 12658 pages reserved
> 0 pages cma reserved
> 0 pages hwpoisoned

From the above you can see that you are pretty much out of memory. There
is basically no pagecache to reclaim and your anon memory is not 
reclaimable either because the swap is basically full. It is true that 
the high atomic reserves consume 19MB which could be reused but this 
less than 1%, especially when you compare that to the amount of reserved
memory.

So while I do agree that potential issues - misaccounting and others you
are addressing in the follow up patch - are good to fix but I believe that
draining last 19M is not something that would reliably get you over the
edge. Your workload (93% of memory sitting on anon LRU with swap full)
simply doesn't fit into the amount of memory you have available.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web