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