Date:   Mon, 20 Jan 2020 11:12:02 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Li Xinhai <lixinhai.lxh@...il.com>
Cc:     "linux-mm@...ck.org" <linux-mm@...ck.org>,
        akpm <akpm@...ux-foundation.org>,
        torvalds <torvalds@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mike Kravetz <mike.kravetz@...cle.com>
Subject: Re: [PATCH v4] mm/mempolicy,hugetlb: Checking hstate for hugetlbfs
 page in vma_migratable

On Sat 18-01-20 11:11:23, Li Xinhai wrote:
> On 2020-01-17 at 11:16 Li Xinhai wrote:
> >On 2020-01-16 at 23:38 Li Xinhai wrote:
> >>On 2020-01-16 at 23:18 Michal Hocko wrote:
> >>>On Thu 16-01-20 21:50:34, Li Xinhai wrote:
> >>>> On 2020-01-16 at 17:56 Michal Hocko wrote:
> >>>> >On Thu 16-01-20 04:11:25, Li Xinhai wrote:
> >>>> >> Checking hstate at early phase when isolating page, instead of during
> >>>> >> unmap and move phase, to avoid useless isolation.
> >>>> >
> >>>> >Could you be more specific what you mean by isolation and why does it
> >>>> >matter? The patch description should really explain _why_ the change is
> >>>> >needed or desirable.
> >>>>
> >>>> The changelog can be improved:
> >>>>
> >>>> vma_migratable() is called to check if pages in vma can be migrated
> >>>> before go ahead to isolate, unmap and move pages. For hugetlb pages,
> >>>> hugepage_migration_supported(struct hstate *h) is one factor which
> >>>> decide if migration is supported. In current code, this function is called
> >>>> from unmap_and_move_huge_page(), after isolating page has
> >>>> completed.
> >>>> This patch checks hstate from vma_migratable() and avoids isolating pages
> >>>> which are not supported.
> >>>
> >>>This still explains what but not why this is relevant. If by isolating
> >>>pages you mean isolate_lru_page then this really a noop for hugetlb
> >>>pages. Or do I still misread your changelog?
> >>
> >>I mean isolate_huge_page will queue pages for moving, and
> >>unmap_and_move_huge_page will call
> >>hugepage_migration_supported then refuse moving.
> >>
> >
> >Forgot to mention that this patch has no relevant with this one
> >https://patchwork.kernel.org/patch/11331639/, 
> >
> >Code change at here is common for avoids walking page table and
> >isolate hugepage in case architecture or page size are not supported
> >for migration. Comments from code are copied here:
> >
> >static int unmap_and_move_huge_page(...)
> >{
> >	/*
> >	* Migratability of hugepages depends on architectures and their size.
> >	* This check is necessary because some callers of hugepage migration
> >	* like soft offline and memory hotremove don't walk through page
> >	* tables or check whether the hugepage is pmd-based or not before
> >	* kicking migration.
> >	*/
> >	if (!hugepage_migration_supported(page_hstate(hpage))) {
> >	putback_active_hugepage(hpage);
> >	return -ENOSYS;
> >	}
> >}
> >
> >For current code change, we are able to know the 'hstate' because we have 'vma', so
> >do early check instead of later.
> > 
> 
> https://lore.kernel.org/linux-mm/20200117111629898234212@gmail.com/
> 
> Revise with more details on changelog for reason why this patch
> is need. Thanks for your comments.
> 
> ---
> vma_migratable() is called to check if pages in vma can be migrated
> before go ahead to further actions. Currently it is used in below code
> path:
> - task_numa_work (kernel\sched\fair.c)
> - mbind (mm\mempolicy.c)
> - move_pages (mm\migrate.c)
> 
> For hugetlb mapping, vma is migratable or not is determined by:
> - CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
> - arch_hugetlb_migration_supported
> 
> Issue: current code only checks for CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION.

Explain why this is an issue. Because as things stand now this doesn't
cause any problems at all. All architectures simply support migrating
all hugetlb sizes AFAIK. If this is not the case then mention which of
them are not.

> This patch checks the two factors to impove code logic. Besides, caller
> of vma_migratable can take action properly in case architecture don't
> support migration, e.g., mbind don't go further to try isolating/moving
> pages, but currently it do continue because vma_migratable say yes.

I do not follow. What are you trying to say here?

The changelog should be explicit it doesn't really fix any existing
problem. And the whole purpose is a code robustness cleanup. Should we
ever have an architecture that doesn't support all hugetlb sizes then
we would safe pointless huge page isolation for a page that cannot be
migrated in the first place.

See how the above actually explains _why_ you want to make the change?

> No adding for Fix tag, since vma_migratable was implemented before
> arch_hugetlb_migration_supported, it is up to the caller to use it.

This is also of no relevance.
-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists