Re: [PATCH 2/2] mm: zero remaining unavailable struct pages

[Masayoshi Mizuma <msys.mizuma@gmail.com>]

On Fri, Aug 31, 2018 at 02:55:36AM +0000, Naoya Horiguchi wrote:
> On Wed, Aug 29, 2018 at 11:16:30AM -0400, Masayoshi Mizuma wrote:
> > Hi Horiguchi-san and Pavel
> > 
> > Thank you for your comments!
> > The Pavel's additional patch looks good to me, so I will add it to this series.
> > 
> > However, unfortunately, the movable_node option has something wrong yet...
> > When I offline the memory which belongs to movable zone, I got the following
> > warning. I'm trying to debug it.
> > 
> > I try to describe the issue as following. 
> > If you have any comments, please let me know.
> > 
> > WARNING: CPU: 156 PID: 25611 at mm/page_alloc.c:7730 has_unmovable_pages+0x1bf/0x200
> > RIP: 0010:has_unmovable_pages+0x1bf/0x200
> > ...
> > Call Trace:
> >  is_mem_section_removable+0xd3/0x160
> >  show_mem_removable+0x8e/0xb0
> >  dev_attr_show+0x1c/0x50
> >  sysfs_kf_seq_show+0xb3/0x110
> >  seq_read+0xee/0x480
> >  __vfs_read+0x36/0x190
> >  vfs_read+0x89/0x130
> >  ksys_read+0x52/0xc0
> >  do_syscall_64+0x5b/0x180
> >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > RIP: 0033:0x7fe7b7823f70
> > ...
> > 
> > I added a printk to catch the unmovable page.
> > ---
> > @@ -7713,8 +7719,12 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> >                  * is set to both of a memory hole page and a _used_ kernel
> >                  * page at boot.
> >                  */
> > -               if (found > count)
> > +               if (found > count) {
> > +                       pr_info("DEBUG: %s zone: %lx page: %lx pfn: %lx flags: %lx found: %ld count: %ld \n",
> > +                               __func__, zone, page, page_to_pfn(page), page->flags, found, count);
> >                         goto unmovable;
> > +               }
> > ---
> > 
> > Then I got the following. The page (PFN: 0x1c0ff130d) flag is 
> > 0xdfffffc0040048 (uptodate|active|swapbacked)
> > 
> > ---
> > DEBUG: has_unmovable_pages zone: 0xffff8c0ffff80380 page: 0xffffea703fc4c340 pfn: 0x1c0ff130d flags: 0xdfffffc0040048 found: 1 count: 0 
> > ---
> > 
> > And I got the owner from /sys/kernel/debug/page_owner.
> > 
> > Page allocated via order 0, mask 0x6280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO)
> > PFN 7532909325 type Movable Block 14712713 type Movable Flags 0xdfffffc0040048(uptodate|active|swapbacked)
> >  __alloc_pages_nodemask+0xfc/0x270
> >  alloc_pages_vma+0x7c/0x1e0
> >  handle_pte_fault+0x399/0xe50
> >  __handle_mm_fault+0x38e/0x520
> >  handle_mm_fault+0xdc/0x210
> >  __do_page_fault+0x243/0x4c0
> >  do_page_fault+0x31/0x130
> >  page_fault+0x1e/0x30
> > 
> > The page is allocated as anonymous page via page fault.
> > I'm not sure, but lru flag should be added to the page...?
> 
> There is a small window of no PageLRU flag just after page allocation
> until the page is linked to some LRU list.
> This kind of unmovability is transient, so retrying can work.
> 
> I guess that this warning seems to be visible since commit 15c30bc09085
> ("mm, memory_hotplug: make has_unmovable_pages more robust")
> which turned off the optimization based on the assumption that pages
> under ZONE_MOVABLE are always movable.
> I think that it helps developers find the issue that permanently
> unmovable pages are accidentally located in ZONE_MOVABLE zone.
> But even ZONE_MOVABLE zone could have transiently unmovable pages,
> so the reported warning seems to me a false charge and should be avoided.
> Doing lru_add_drain_all()/drain_all_pages() before has_unmovable_pages()
> might be helpful?

Thanks you for your proposal! And sorry for delayed responce.

lru_add_drain_all()/drain_all_pages() might be helpful, but it 
seems that the window is not very small because I tried to do
offline some times, and every offline failed...

I have another idea. I found that if the page is belonged to
Movable zone and it has Uptodate flag, the page will go lru
soon, so I think we can pass the page.
Does the idea make sence? As far as I tested it, it works well.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 52d9efe8c9fb..ecf87bec8ac6 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7758,6 +7758,9 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
                if (__PageMovable(page))
                        continue;

+               if ((zone_idx(zone) == ZONE_MOVABLE) && PageUptodate(page))
+                       continue;
+
                if (!PageLRU(page))
                        found++;
                /*

Thanks,
Masa