Re: [PATCH v4] mm/mempolicy,hugetlb: Checking hstate for hugetlbfs page in vma_migratable

Re: [PATCH v4] mm/mempolicy,hugetlb: Checking hstate for hugetlbfs page in vma_migratable

[Li Xinhai]

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.

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.

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


>>>--
>>>Michal Hocko
>>>SUSE Labs

Re: [PATCH v4] mm/mempolicy,hugetlb: Checking hstate for hugetlbfs page in vma_migratable

[Michal Hocko]

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

Re: [PATCH v4] mm/mempolicy,hugetlb: Checking hstate for hugetlbfs page in vma_migratable

[Michal Hocko]

On Mon 20-01-20 23:37:25, Li Xinhai wrote:
[...]
> Changelog is updated as below, thanks for comments:
> ---
> mm/mempolicy: Checking hugepage migration is supported by arch in vma_migratable
> 
> 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
> - mbind
> - move_pages
> 
> For hugetlb mapping, whether 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,
> which express less accurate semantics of vma_migratable(). (note that
> current code in vma_migratable don't cause failure or bug because
> unmap_and_move_huge_page() will catch unsupported hugepage and handle it
> properly)
> 
> This patch checks the two factors for impoveing code logic and
> robustness. It will enable early bail out of hugepage migration procedure,
> but because currently all architecture supporting hugepage migration is able
> to support all page size, we would not see performance gain with this patch
> applied.

This looks definitely better than the original one. I hope it is more
clear to you what I meant by a better description for the justification.
I would just add that the no code should use
CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION directly and use
arch_hugetlb_migration_supported instead. This will be the case after
this patch.

Please keep in mind that changelogs are really important and growing in
importance as the code gets more complicated over time. It is much more
easier to see what the patch does because reading diffs and the code is
easy but the lack of motivation is what people usually fighting with.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v4] mm/mempolicy,hugetlb: Checking hstate for hugetlbfs page in vma_migratable

[Anshuman Khandual]

On 01/20/2020 09:35 PM, Michal Hocko wrote:
> On Mon 20-01-20 23:37:25, Li Xinhai wrote:
> [...]
>> Changelog is updated as below, thanks for comments:
>> ---
>> mm/mempolicy: Checking hugepage migration is supported by arch in vma_migratable
>>
>> 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
>> - mbind
>> - move_pages
>>
>> For hugetlb mapping, whether 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,
>> which express less accurate semantics of vma_migratable(). (note that
>> current code in vma_migratable don't cause failure or bug because
>> unmap_and_move_huge_page() will catch unsupported hugepage and handle it
>> properly)
>>
>> This patch checks the two factors for impoveing code logic and
>> robustness. It will enable early bail out of hugepage migration procedure,
>> but because currently all architecture supporting hugepage migration is able
>> to support all page size, we would not see performance gain with this patch
>> applied.
> 
> This looks definitely better than the original one. I hope it is more
> clear to you what I meant by a better description for the justification.
> I would just add that the no code should use
> CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION directly and use
> arch_hugetlb_migration_supported instead. This will be the case after
> this patch.

As I have mentioned previously on the other thread, there might be an case
to keep the existing code (just added with a comment) which will preserve
the performance. But the proposed method will do it the right way and also
get rid of CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION here. Its OK either way.

> 
> Please keep in mind that changelogs are really important and growing in
> importance as the code gets more complicated over time. It is much more
> easier to see what the patch does because reading diffs and the code is
> easy but the lack of motivation is what people usually fighting with.
>