On Mon 20-01-20 14:51:31, Anshuman Khandual wrote: > > > On 01/16/2020 08:48 PM, 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? > > unmap_and_move_hugepage() aborts migrating a HugeTLB page (from the list) > if it's corresponding hstate does not support migration. But all architectures support all hugeltb sizes unless I am missing something. If there is some which doesn't then the changelog should mention that. I have already asked for runtime effects with no data provided. Just to make it clear. I am not objecting to the patch itself. I am objecting to the very vague justification. The changelog doesn't explain _why_ do we need to change this. Is it a bug, non-optimal code, pure code clean up for a more robust code? -- Michal Hocko SUSE Labs
