On Mon, May 30, 2016 at 11:36:07AM +0200, Vlastimil Babka wrote:
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() ..."?
It's better. I will change it.
--- 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.
To me, __ClearPageMovable naming is suitable for user POV. It effectively makes the page unmovable. The confusion is just caused by the implementation and I don't prefer exported API depends on the implementation. So I want to add just comment.
I didn't add comment above the function because I don't want to export internal implementation to the user. I think they don't need to know it.
index a7df2ae71f2a..d1d2063b4fd9 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -108,6 +108,11 @@ void __ClearPageMovable(struct page *page) { VM_BUG_ON_PAGE(!PageLocked(page), page); VM_BUG_ON_PAGE(!PageMovable(page), page); + /* + * Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE + * flag so that VM can catch up released page by driver after isolation. + * With it, VM migration doesn't try to put it back. + */ page->mapping = (void *)((unsigned long)page->mapping & PAGE_MAPPING_MOVABLE);
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?
I don't understand. Yeah, all caller are not under page lock but at least, new user of movable pages should call it under page_lock. Yeah, I will write the rule down in document. In this case, what kinds of problem do you see?
- 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)