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

Groups > linux.kernel > #1463549 > unrolled thread

Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct page init

Started byMichal Hocko <mhocko@kernel.org>
First post2016-08-16 10:50 +0200
Last post2016-08-16 13:20 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct  page init Michal Hocko <mhocko@kernel.org> - 2016-08-16 10:50 +0200
    Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct  page init Xishi Qiu <qiuxishi@huawei.com> - 2016-08-16 11:00 +0200
      Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct  page init Michal Hocko <mhocko@kernel.org> - 2016-08-16 11:30 +0200
        [PATCH v2] mm: fix set pageblock migratetype in deferred struct page  init Xishi Qiu <qiuxishi@huawei.com> - 2016-08-16 12:10 +0200
        Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct  page init Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 12:20 +0200
          Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct  page init Xishi Qiu <qiuxishi@huawei.com> - 2016-08-16 12:30 +0200
          Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct  page init Michal Hocko <mhocko@kernel.org> - 2016-08-16 13:20 +0200

#1463549 — Re: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct page init

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-16 10:50 +0200
SubjectRe: [PATCH 1/3] mm: fix set pageblock migratetype in deferred struct page init
Message-ID<s6Iem-Nx-13@gated-at.bofh.it>
On Thu 04-08-16 19:25:03, Xishi Qiu wrote:
> MAX_ORDER_NR_PAGES is usually 4M, and a pageblock is usually 2M, so we only
> set one pageblock's migratetype in deferred_free_range() if pfn is aligned
> to MAX_ORDER_NR_PAGES.

Do I read the changelog correctly and the bug causes leaking unmovable
allocations into movable zones?
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1463557

FromXishi Qiu <qiuxishi@huawei.com>
Date2016-08-16 11:00 +0200
Message-ID<s6Io1-R5-13@gated-at.bofh.it>
In reply to#1463549
On 2016/8/16 16:41, Michal Hocko wrote:

> On Thu 04-08-16 19:25:03, Xishi Qiu wrote:
>> MAX_ORDER_NR_PAGES is usually 4M, and a pageblock is usually 2M, so we only
>> set one pageblock's migratetype in deferred_free_range() if pfn is aligned
>> to MAX_ORDER_NR_PAGES.
> 
> Do I read the changelog correctly and the bug causes leaking unmovable
> allocations into movable zones?

Hi Michal,

This bug will cause uninitialized migratetype, you can see from
"cat /proc/pagetypeinfo", almost half blocks are Unmovable.

Also this bug missed to free the last block pages, it cause memory leaking.

Thanks,
Xishi Qiu

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

#1463588

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-16 11:30 +0200
Message-ID<s6IR4-1hW-13@gated-at.bofh.it>
In reply to#1463557
On Tue 16-08-16 16:56:54, Xishi Qiu wrote:
> On 2016/8/16 16:41, Michal Hocko wrote:
> 
> > On Thu 04-08-16 19:25:03, Xishi Qiu wrote:
> >> MAX_ORDER_NR_PAGES is usually 4M, and a pageblock is usually 2M, so we only
> >> set one pageblock's migratetype in deferred_free_range() if pfn is aligned
> >> to MAX_ORDER_NR_PAGES.
> > 
> > Do I read the changelog correctly and the bug causes leaking unmovable
> > allocations into movable zones?
> 
> Hi Michal,
> 
> This bug will cause uninitialized migratetype, you can see from
> "cat /proc/pagetypeinfo", almost half blocks are Unmovable.

Please add that information to the changelog. Leaking unmovable
allocations to the movable zones defeats the whole purpose of the
movable zone so I guess we really want to mark this for stable.
AFAICS it should also note:
Fixes: ac5d2539b238 ("mm: meminit: reduce number of times pageblocks are set during struct page init")
and stable 4.2+

-- 
Michal Hocko
SUSE Labs

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

#1463643 — [PATCH v2] mm: fix set pageblock migratetype in deferred struct page init

FromXishi Qiu <qiuxishi@huawei.com>
Date2016-08-16 12:10 +0200
Subject[PATCH v2] mm: fix set pageblock migratetype in deferred struct page init
Message-ID<s6JtM-1KE-19@gated-at.bofh.it>
In reply to#1463588
Fixes: ac5d2539b238 ("mm: meminit: reduce number of times pageblocks are set during struct page init")
and stable 4.2+

on x86_64 MAX_ORDER_NR_PAGES is usually 4M, and a pageblock is usually 2M,
so we only set one pageblock's migratetype in deferred_free_range() if pfn
is aligned to MAX_ORDER_NR_PAGES. That means it causes uninitialized migratetype
blocks, you can see from "cat /proc/pagetypeinfo", almost half blocks are
Unmovable.

Also we missed to free the last block in deferred_init_memmap(), it causes
memory leak.

Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
---
 mm/page_alloc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2b258ec..e0ec3b6 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1399,15 +1399,18 @@ static void __init deferred_free_range(struct page *page,
 		return;
 
 	/* Free a large naturally-aligned chunk if possible */
-	if (nr_pages == MAX_ORDER_NR_PAGES &&
-	    (pfn & (MAX_ORDER_NR_PAGES-1)) == 0) {
+	if (nr_pages == pageblock_nr_pages &&
+	    (pfn & (pageblock_nr_pages - 1)) == 0) {
 		set_pageblock_migratetype(page, MIGRATE_MOVABLE);
-		__free_pages_boot_core(page, MAX_ORDER-1);
+		__free_pages_boot_core(page, pageblock_order);
 		return;
 	}
 
-	for (i = 0; i < nr_pages; i++, page++)
+	for (i = 0; i < nr_pages; i++, page++, pfn++) {
+		if ((pfn & (pageblock_nr_pages - 1)) == 0)
+			set_pageblock_migratetype(page, MIGRATE_MOVABLE);
 		__free_pages_boot_core(page, 0);
+	}
 }
 
 /* Completion tracking for deferred_init_memmap() threads */
@@ -1475,9 +1478,9 @@ static int __init deferred_init_memmap(void *data)
 
 			/*
 			 * Ensure pfn_valid is checked every
-			 * MAX_ORDER_NR_PAGES for memory holes
+			 * pageblock_nr_pages for memory holes
 			 */
-			if ((pfn & (MAX_ORDER_NR_PAGES - 1)) == 0) {
+			if ((pfn & (pageblock_nr_pages - 1)) == 0) {
 				if (!pfn_valid(pfn)) {
 					page = NULL;
 					goto free_range;
@@ -1490,7 +1493,7 @@ static int __init deferred_init_memmap(void *data)
 			}
 
 			/* Minimise pfn page lookups and scheduler checks */
-			if (page && (pfn & (MAX_ORDER_NR_PAGES - 1)) != 0) {
+			if (page && (pfn & (pageblock_nr_pages - 1)) != 0) {
 				page++;
 			} else {
 				nr_pages += nr_to_free;
@@ -1526,6 +1529,9 @@ free_range:
 			free_base_page = NULL;
 			free_base_pfn = nr_to_free = 0;
 		}
+		/* Free the last block of pages to allocator */
+		nr_pages += nr_to_free;
+		deferred_free_range(free_base_page, free_base_pfn, nr_to_free);
 
 		first_init_pfn = max(end_pfn, first_init_pfn);
 	}
-- 
1.8.3.1

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

#1463645

FromVlastimil Babka <vbabka@suse.cz>
Date2016-08-16 12:20 +0200
Message-ID<s6JDr-1NW-15@gated-at.bofh.it>
In reply to#1463588
On 08/16/2016 11:23 AM, Michal Hocko wrote:
> On Tue 16-08-16 16:56:54, Xishi Qiu wrote:
>> On 2016/8/16 16:41, Michal Hocko wrote:
>>
>>> On Thu 04-08-16 19:25:03, Xishi Qiu wrote:
>>>> MAX_ORDER_NR_PAGES is usually 4M, and a pageblock is usually 2M, so we only
>>>> set one pageblock's migratetype in deferred_free_range() if pfn is aligned
>>>> to MAX_ORDER_NR_PAGES.
>>>
>>> Do I read the changelog correctly and the bug causes leaking unmovable
>>> allocations into movable zones?
>>
>> Hi Michal,
>>
>> This bug will cause uninitialized migratetype, you can see from
>> "cat /proc/pagetypeinfo", almost half blocks are Unmovable.
>
> Please add that information to the changelog. Leaking unmovable
> allocations to the movable zones defeats the whole purpose of the
> movable zone so I guess we really want to mark this for stable.

Note that it's not as severe. Pageblock migratetype is just heuristic 
against fragmentation. It should not allow unmovable allocations from 
movable zones (although I can't find what really does govern it).

> AFAICS it should also note:
> Fixes: ac5d2539b238 ("mm: meminit: reduce number of times pageblocks are set during struct page init")
> and stable 4.2+

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

#1463648

FromXishi Qiu <qiuxishi@huawei.com>
Date2016-08-16 12:30 +0200
Message-ID<s6JN7-1Ri-1@gated-at.bofh.it>
In reply to#1463645
On 2016/8/16 18:12, Vlastimil Babka wrote:

> On 08/16/2016 11:23 AM, Michal Hocko wrote:
>> On Tue 16-08-16 16:56:54, Xishi Qiu wrote:
>>> On 2016/8/16 16:41, Michal Hocko wrote:
>>>
>>>> On Thu 04-08-16 19:25:03, Xishi Qiu wrote:
>>>>> MAX_ORDER_NR_PAGES is usually 4M, and a pageblock is usually 2M, so we only
>>>>> set one pageblock's migratetype in deferred_free_range() if pfn is aligned
>>>>> to MAX_ORDER_NR_PAGES.
>>>>
>>>> Do I read the changelog correctly and the bug causes leaking unmovable
>>>> allocations into movable zones?
>>>
>>> Hi Michal,
>>>
>>> This bug will cause uninitialized migratetype, you can see from
>>> "cat /proc/pagetypeinfo", almost half blocks are Unmovable.
>>
>> Please add that information to the changelog. Leaking unmovable
>> allocations to the movable zones defeats the whole purpose of the
>> movable zone so I guess we really want to mark this for stable.
> 
> Note that it's not as severe. Pageblock migratetype is just heuristic against fragmentation. It should not allow unmovable allocations from movable zones (although I can't find what really does govern it).
> 

Yes, leaking unmovable migratetype to movable zone is fine for mem-offline,
we will check every page in offline_pages().
But as I pointed that we missed to free the last block in deferred_init_memmap(),
this will lead to mem-offline fail.

Thanks,
Xishi Qiu

>> AFAICS it should also note:
>> Fixes: ac5d2539b238 ("mm: meminit: reduce number of times pageblocks are set during struct page init")
>> and stable 4.2+
> 
> 
> 
> 
> .
>

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

#1463704

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-16 13:20 +0200
Message-ID<s6Kzv-2mV-9@gated-at.bofh.it>
In reply to#1463645
On Tue 16-08-16 12:12:07, Vlastimil Babka wrote:
> On 08/16/2016 11:23 AM, Michal Hocko wrote:
> > On Tue 16-08-16 16:56:54, Xishi Qiu wrote:
> > > On 2016/8/16 16:41, Michal Hocko wrote:
> > > 
> > > > On Thu 04-08-16 19:25:03, Xishi Qiu wrote:
> > > > > MAX_ORDER_NR_PAGES is usually 4M, and a pageblock is usually 2M, so we only
> > > > > set one pageblock's migratetype in deferred_free_range() if pfn is aligned
> > > > > to MAX_ORDER_NR_PAGES.
> > > > 
> > > > Do I read the changelog correctly and the bug causes leaking unmovable
> > > > allocations into movable zones?
> > > 
> > > Hi Michal,
> > > 
> > > This bug will cause uninitialized migratetype, you can see from
> > > "cat /proc/pagetypeinfo", almost half blocks are Unmovable.
> > 
> > Please add that information to the changelog. Leaking unmovable
> > allocations to the movable zones defeats the whole purpose of the
> > movable zone so I guess we really want to mark this for stable.
> 
> Note that it's not as severe. Pageblock migratetype is just heuristic
> against fragmentation. It should not allow unmovable allocations from
> movable zones (although I can't find what really does govern it).

You are right! gfp_zone would disabllow movable zones from the zone
list. So we indeed cannot leak the unmovable allocation to the movable
zone and then this doesn't really sound all that important to bother
with stable backport. It would be really great to have this all in the
changelog. This code is far from straightforward so having some
assistance from the changelog is more than welcome.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web