On 05/30/2016 03:33 AM, Minchan Kim wrote:
- page->mapping = (void *)((unsigned long)page->mapping &
PAGE_MAPPING_MOVABLE);This should be negated to clear... use ~PAGE_MAPPING_MOVABLE ?
No.
The intention is to clear only mapping value but PAGE_MAPPING_MOVABLE flag. So, any new migration trial will be failed because PageMovable checks page's mapping value but ongoing migraion handling can catch whether it's movable page or not with the type bit.
Oh, OK, I got that wrong. I'll point out in the reply to the v6v2 what misled me :)
So this effectively prevents movable compound pages from being migrated. Are you sure no users of this functionality are going to have compound pages? I assumed that they could, and so made the code like this, with the is_lru variable (which is redundant after your change).
This implementation at the moment disables effectively non-lru compound page migration but I'm not a god so I can't make sure no one doesn't want it in future. If someone want it, we can support it then because this work doesn't prevent it by design.
Oh well. As long as the balloon pages or zsmalloc don't already use compound pages...
I thouht PageCompound check right before isolate_movable_page in isolate_migratepages_block will filter it out mostly but yeah it is racy without zone->lru_lock so it could reach to isolate_movable_page. However, PageMovable check in there investigates mapping, mapping->a_ops, and a_ops->isolate_page to verify whether it's movable page or not.
I thought it's sufficient to filter THP page.
I guess, yeah.
[...]
@@ -755,33 +844,69 @@ static int move_to_new_page(struct page *newpage, struct page *page, enum migrate_mode mode) { struct address_space *mapping;
- int rc;
int rc = -EAGAIN;
bool is_lru = !__PageMovable(page);
VM_BUG_ON_PAGE(!PageLocked(page), page); VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
mapping = page_mapping(page);
- if (!mapping)
rc = migrate_page(mapping, newpage, page, mode);- else if (mapping->a_ops->migratepage)
/** Most pages have a mapping and most filesystems provide a* migratepage callback. Anonymous pages are part of swap* space which also has its own migratepage callback. This* is the most common path for page migration.*/rc = mapping->a_ops->migratepage(mapping, newpage, page, mode);- else
rc = fallback_migrate_page(mapping, newpage, page, mode);
- /*
* In case of non-lru page, it could be released after* isolation step. In that case, we shouldn't try* fallback migration which is designed for LRU pages.*/Hmm but is_lru was determined from !__PageMovable() above, also well after the isolation step. So if the driver already released it, we wouldn't detect it? And this function is all under same page lock, so if __PageMovable was true above, so will be PageMovable below?
You are missing what I mentioned above. We should keep the type bit to catch what you are saying(i.e., driver already released).
__PageMovable just checks PAGE_MAPPING_MOVABLE flag and PageMovable checks page->mapping valid while __ClearPageMovable reset only valid vaule of mapping, not PAGE_MAPPING_MOVABLE flag.
I wrote it down in Documentation/vm/page_migration.
"For testing of non-lru movable page, VM supports __PageMovable function. However, it doesn't guarantee to identify non-lru movable page because page->mapping field is unified with other variables in struct page. As well, if driver releases the page after isolation by VM, page->mapping doesn't have stable value although it has PAGE_MAPPING_MOVABLE (Look at __ClearPageMovable). But __PageMovable is cheap to catch whether page is LRU or non-lru movable once the page has been isolated. Because LRU pages never can have PAGE_MAPPING_MOVABLE in page->mapping. It is also good for just peeking to test non-lru movable pages before more expensive checking with lock_page in pfn scanning to select victim.
For guaranteeing non-lru movable page, VM provides PageMovable function. Unlike __PageMovable, PageMovable functions validates page->mapping and mapping->a_ops->isolate_page under lock_page. The lock_page prevents sudden destroying of page->mapping.
Driver using __SetPageMovable should clear the flag via __ClearMovablePage under page_lock before the releasing the page."
Right, I get it now.
if (!((unsigned long)page->mapping & PAGE_MAPPING_FLAGS)) page->mapping = NULL;The two lines above make little sense to me without a comment.
I folded this.
@@ -901,7 +901,12 @@ static int move_to_new_page(struct page *newpage, struct page *page, __ClearPageIsolated(page); }
if (!((unsigned long)page->mapping & PAGE_MAPPING_FLAGS))
/** Anonymous and movable page->mapping will be cleard by* free_pages_prepare so don't reset it here for keeping* the type to work PageAnon, for example.*/if (!PageMappingFlags(page)) page->mapping = NULL; }
Thanks.