Re: [PATCH v2] mm, thp: use head page in __migration_entry_wait

[Kirill A. Shutemov]

On Tue, Jun 08, 2021 at 09:22:28PM +0800, Yu Xu wrote:
> On 6/8/21 8:00 PM, Kirill A. Shutemov wrote:
> > On Tue, Jun 08, 2021 at 05:22:39PM +0800, Xu Yu wrote:
> > > We notice that hung task happens in a conner but practical scenario when
> > > CONFIG_PREEMPT_NONE is enabled, as follows.
> > > 
> > > Process 0                       Process 1                     Process 2..Inf
> > > split_huge_page_to_list
> > >      unmap_page
> > >          split_huge_pmd_address
> > >                                  __migration_entry_wait(head)
> > >                                                                __migration_entry_wait(tail)
> > >      remap_page (roll back)
> > >          remove_migration_ptes
> > >              rmap_walk_anon
> > >                  cond_resched
> > > 
> > > Where __migration_entry_wait(tail) is occurred in kernel space, e.g.,
> > > copy_to_user in fstat, which will immediately fault again without
> > > rescheduling, and thus occupy the cpu fully.
> > > 
> > > When there are too many processes performing __migration_entry_wait on
> > > tail page, remap_page will never be done after cond_resched.
> > > 
> > > This makes __migration_entry_wait operate on the compound head page,
> > > thus waits for remap_page to complete, whether the THP is split
> > > successfully or roll back.
> > > 
> > > Note that put_and_wait_on_page_locked helps to drop the page reference
> > > acquired with get_page_unless_zero, as soon as the page is on the wait
> > > queue, before actually waiting. So splitting the THP is only prevented
> > > for a brief interval.
> > > 
> > > Fixes: ba98828088ad ("thp: add option to setup migration entries during PMD split")
> > > Suggested-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > > Signed-off-by: Gang Deng <gavin.dg@xxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Xu Yu <xuyu@xxxxxxxxxxxxxxxxx>
> > 
> > Looks good to me:
> > 
> > Acked-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > 
> > But there's one quirk: if split succeed we effectively wait on wrong
> > page to be unlocked. And it may take indefinite time if split_huge_page()
> > was called on the head page.
> 
> Inspired by you, I look into the codes, and have a new question (nothing
> to do with this patch).
> 
> If we split_huge_page_to_list on *tail* page (in fact, I haven't seen
> that used yet),

See ksm code for instance.

> mm/huge_memory.c:2666 checks "VM_BUG_ON_PAGE(!PageLocked(head), head);"
> in split_huge_page_to_list(), while
> 
> mm/huge_memory.c:2497 does "unlock_page(subpage)", where subpage can
> be head in this scenario, in __split_huge_page().
> 
> My confusion is
> 1) how the pin on the @subpage is got outside split_huge_page_to_list()?
>    can we ever get tail page?

This loop:

	for (i = 0; i < nr; i++) {
		struct page *subpage = head + i;
		if (subpage == page)
			continue;
		unlock_page(subpage);

		/*
		 * Subpages may be freed if there wasn't any mapping
		 * like if add_to_swap() is running on a lru page that
		 * had its mapping zapped. And freeing these pages
		 * requires taking the lru_lock so we do the put_page
		 * of the tail pages after the split is complete.
		 */
		put_page(subpage);
	}

We skip unlocking and unpinning the page split_huge_page() got called for.

> 
> 2) head page is locked outside split_huge_page_to_list(), but unlocked
>    in __split_huge_page()?

If called on tail page, yes.

-- 
 Kirill A. Shutemov