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

["Li Xinhai" <lixinhai.lxh@gmail.com>]

On 2020-01-21 at 12:12 Anshuman Khandual wrote:
>
>
>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.
> 

In my understanding, we need to starting utilize the exported arch* interface,
insteadd of till some arch support only part of page size, that would be hard
for arch developers to notitce that there is a site want to call it, and add the
call. (BTW, arch_hugetlb_migration_supported also give arch the right to decide
whether migration is supported or not by more factors, besides page size)

Yes, CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION is removed. And I provide
VMA level checking with a new interface for hugetlb mapping, so later if we
need add more checks, that will also help us, and lower the chance for error.

>>
>> 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.
>>