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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>