On 05/30/2016 03:39 AM, Minchan Kim wrote: > After isolation, VM calls migratepage of driver with isolated page. > The function of migratepage is to move content of the old page to new page > and set up fields of struct page newpage. Keep in mind that you should > clear PG_movable of oldpage via __ClearPageMovable under page_lock if you > migrated the oldpage successfully and returns 0.
This "clear PG_movable" is one of the reasons I was confused about what __ClearPageMovable() really does. There's no actual "PG_movable" page flag and the function doesn't clear even the actual mapping flag :) Also same thing in the Documentation/ part. Something like "... you should indicate to the VM that the oldpage is no longer movable via __ClearPageMovable() ..."? > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -81,6 +81,39 @@ static inline bool migrate_async_suitable(int migratetype) > > #ifdef CONFIG_COMPACTION > > +int PageMovable(struct page *page) > +{ > + struct address_space *mapping; > + > + VM_BUG_ON_PAGE(!PageLocked(page), page); > + if (!__PageMovable(page)) > + return 0; > + > + mapping = page_mapping(page); > + if (mapping && mapping->a_ops && mapping->a_ops->isolate_page) > + return 1; > + > + return 0; > +} > +EXPORT_SYMBOL(PageMovable); > + > +void __SetPageMovable(struct page *page, struct address_space *mapping) > +{ > + VM_BUG_ON_PAGE(!PageLocked(page), page); > + VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page); > + page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE); > +} > +EXPORT_SYMBOL(__SetPageMovable); > + > +void __ClearPageMovable(struct page *page) > +{ > + VM_BUG_ON_PAGE(!PageLocked(page), page); > + VM_BUG_ON_PAGE(!PageMovable(page), page); > + page->mapping = (void *)((unsigned long)page->mapping & > + PAGE_MAPPING_MOVABLE); > +} > +EXPORT_SYMBOL(__ClearPageMovable); The second confusing thing is that the function is named __ClearPageMovable(), but what it really clears is the mapping pointer, which is not at all the opposite of what __SetPageMovable() does. I know it's explained in the documentation, but it also deserves a comment here so it doesn't confuse everyone who looks at it. Even better would be a less confusing name for the function, but I can't offer one right now. > diff --git a/mm/util.c b/mm/util.c > index 917e0e3d0f8e..b756ee36f7f0 100644 > --- a/mm/util.c > +++ b/mm/util.c > @@ -399,10 +399,12 @@ struct address_space *page_mapping(struct page *page) > } > > mapping = page->mapping; I'd probably use READ_ONCE() here to be safe. Not all callers are under page lock? > - if ((unsigned long)mapping & PAGE_MAPPING_FLAGS) > + if ((unsigned long)mapping & PAGE_MAPPING_ANON) > return NULL; > - return mapping; > + > + return (void *)((unsigned long)mapping & ~PAGE_MAPPING_FLAGS); > } > +EXPORT_SYMBOL(page_mapping); > > /* Slow path of page_mapcount() for compound pages */ > int __page_mapcount(struct page *page) >