On Wed 22-11-17 04:18:35, Zi Yan wrote:
On 22 Nov 2017, at 3:54, Michal Hocko wrote:
[...]
I would keep the two checks consistent. But that leads to a more interesting question. new_page_nodemask does
if (thp_migration_supported() && PageTransHuge(page)) { order = HPAGE_PMD_ORDER; gfp_mask |= GFP_TRANSHUGE; }
How come it is safe to allocate an order-0 page if !thp_migration_supported() when we are about to migrate THP? This doesn't make any sense to me. Are we working around this somewhere else? Why shouldn't we simply return NULL here?
If !thp_migration_supported(), we will first split a THP and migrate its head page. This process is done in unmap_and_move() after get_new_page() (the function pointer to this new_page_nodemask()) is called. The situation can be PageTransHuge(page) is true here, but the page is split in unmap_and_move(), so we want to return a order-0 page here.
This deserves a big fat comment in the code because this is not clear from the code!
I think the confusion comes from that there is no guarantee of THP allocation when we are doing THP migration. If we can allocate a THP during THP migration, we are good. Otherwise, we want to fallback to the old way, splitting the original THP and migrating the head page, to preserve the original code behavior.
I understand that but that should be done explicitly rather than relying on two functions doing the right thing because this is just too fragile.
Moreover I am not really sure this is really working properly. Just look at the split_huge_page. It moves all the tail pages to the LRU list while migrate_pages has a list of pages to migrate. So we will migrate the head page and all the rest will get back to the LRU list. What guarantees that they will get migrated as well.
This all looks like a mess!