| Message ID | 1579147885-23511-1-git-send-email-lixinhai.lxh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] mm/mempolicy,hugetlb: Checking hstate for hugetlbfs page in vma_migratable | expand |
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. > Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Mike Kravetz <mike.kravetz@oracle.com> > --- > include/linux/hugetlb.h | 10 ++++++++++ > include/linux/mempolicy.h | 29 +---------------------------- > mm/mempolicy.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 39 insertions(+), 28 deletions(-) > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index 31d4920..c9d871d 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -598,6 +598,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) > return arch_hugetlb_migration_supported(h); > } > > +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) > +{ > + return hugepage_migration_supported(hstate_vma(vma)); > +} > + > /* > * Movability check is different as compared to migration check. > * It determines whether or not a huge page should be placed on > @@ -809,6 +814,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) > return false; > } > > +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) > +{ > + return false; > +} > + > static inline bool hugepage_movable_supported(struct hstate *h) > { > return false; > diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h > index 5228c62..8165278 100644 > --- a/include/linux/mempolicy.h > +++ b/include/linux/mempolicy.h > @@ -173,34 +173,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from, > extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol); > > /* Check if a vma is migratable */ > -static inline bool vma_migratable(struct vm_area_struct *vma) > -{ > - if (vma->vm_flags & (VM_IO | VM_PFNMAP)) > - return false; > - > - /* > - * DAX device mappings require predictable access latency, so avoid > - * incurring periodic faults. > - */ > - if (vma_is_dax(vma)) > - return false; > - > -#ifndef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION > - if (vma->vm_flags & VM_HUGETLB) > - return false; > -#endif > - > - /* > - * Migration allocates pages in the highest zone. If we cannot > - * do so then migration (at least from node to node) is not > - * possible. > - */ > - if (vma->vm_file && > - gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) > - < policy_zone) > - return false; > - return true; > -} > +extern bool vma_migratable(struct vm_area_struct *vma); > > extern int mpol_misplaced(struct page *, struct vm_area_struct *, unsigned long); > extern void mpol_put_task_policy(struct task_struct *); > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 067cf7d..8a01fb1 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1714,6 +1714,34 @@ static int kernel_get_mempolicy(int __user *policy, > > #endif /* CONFIG_COMPAT */ > > +bool vma_migratable(struct vm_area_struct *vma) > +{ > + if (vma->vm_flags & (VM_IO | VM_PFNMAP)) > + return false; > + > + /* > + * DAX device mappings require predictable access latency, so avoid > + * incurring periodic faults. > + */ > + if (vma_is_dax(vma)) > + return false; > + > + if (is_vm_hugetlb_page(vma) && > + !vm_hugepage_migration_supported(vma)) > + return false; > + > + /* > + * Migration allocates pages in the highest zone. If we cannot > + * do so then migration (at least from node to node) is not > + * possible. > + */ > + if (vma->vm_file && > + gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) > + < policy_zone) > + return false; > + return true; > +} > + > struct mempolicy *__get_vma_policy(struct vm_area_struct *vma, > unsigned long addr) > { > -- > 1.8.3.1 >
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. > >> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Mike Kravetz <mike.kravetz@oracle.com> >> --- >> include/linux/hugetlb.h | 10 ++++++++++ >> include/linux/mempolicy.h | 29 +---------------------------- >> mm/mempolicy.c | 28 ++++++++++++++++++++++++++++ >> 3 files changed, 39 insertions(+), 28 deletions(-) >> >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >> index 31d4920..c9d871d 100644 >> --- a/include/linux/hugetlb.h >> +++ b/include/linux/hugetlb.h >> @@ -598,6 +598,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) >> return arch_hugetlb_migration_supported(h); >> } >> >> +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) >> +{ >> + return hugepage_migration_supported(hstate_vma(vma)); >> +} >> + >> /* >> * Movability check is different as compared to migration check. >> * It determines whether or not a huge page should be placed on >> @@ -809,6 +814,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) >> return false; >> } >> >> +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) >> +{ >> + return false; >> +} >> + >> static inline bool hugepage_movable_supported(struct hstate *h) >> { >> return false; >> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h >> index 5228c62..8165278 100644 >> --- a/include/linux/mempolicy.h >> +++ b/include/linux/mempolicy.h >> @@ -173,34 +173,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from, >> extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol); >> >> /* Check if a vma is migratable */ >> -static inline bool vma_migratable(struct vm_area_struct *vma) >> -{ >> - if (vma->vm_flags & (VM_IO | VM_PFNMAP)) >> - return false; >> - >> - /* >> - * DAX device mappings require predictable access latency, so avoid >> - * incurring periodic faults. >> - */ >> - if (vma_is_dax(vma)) >> - return false; >> - >> -#ifndef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION >> - if (vma->vm_flags & VM_HUGETLB) >> - return false; >> -#endif >> - >> - /* >> - * Migration allocates pages in the highest zone. If we cannot >> - * do so then migration (at least from node to node) is not >> - * possible. >> - */ >> - if (vma->vm_file && >> - gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) >> - < policy_zone) >> - return false; >> - return true; >> -} >> +extern bool vma_migratable(struct vm_area_struct *vma); >> >> extern int mpol_misplaced(struct page *, struct vm_area_struct *, unsigned long); >> extern void mpol_put_task_policy(struct task_struct *); >> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >> index 067cf7d..8a01fb1 100644 >> --- a/mm/mempolicy.c >> +++ b/mm/mempolicy.c >> @@ -1714,6 +1714,34 @@ static int kernel_get_mempolicy(int __user *policy, >> >> #endif /* CONFIG_COMPAT */ >> >> +bool vma_migratable(struct vm_area_struct *vma) >> +{ >> + if (vma->vm_flags & (VM_IO | VM_PFNMAP)) >> + return false; >> + >> + /* >> + * DAX device mappings require predictable access latency, so avoid >> + * incurring periodic faults. >> + */ >> + if (vma_is_dax(vma)) >> + return false; >> + >> + if (is_vm_hugetlb_page(vma) && >> + !vm_hugepage_migration_supported(vma)) >> + return false; >> + >> + /* >> + * Migration allocates pages in the highest zone. If we cannot >> + * do so then migration (at least from node to node) is not >> + * possible. >> + */ >> + if (vma->vm_file && >> + gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) >> + < policy_zone) >> + return false; >> + return true; >> +} >> + >> struct mempolicy *__get_vma_policy(struct vm_area_struct *vma, >> unsigned long addr) >> { >> -- >> 1.8.3.1 >> > >-- >Michal Hocko >SUSE Labs
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?
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. >-- >Michal Hocko >SUSE Labs
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. >>-- >>Michal Hocko >>SUSE Labs
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
On 2020-01-18 at 11:11 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/1579147885-23511-1-git-send-email-lixinhai.lxh@gmail.com/ Add context information >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 - arch_hugetlb_migration_supported [1] > >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. [1] This interface is introduced by commit e693de186414 (mm/hugetlb: enable arch specific huge page size support for migration), which allow improvement of this path. >--- > > >>>>-- >>>>Michal Hocko >>>>SUSE Labs
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. IIUC the current proposal will enable early bail out and prevent migration via migrate_pages() at a much higher level like mbind() and other system calls if corresponding VMA is HugeTLB but it's hstate lacks migration support. This should probably save some cycles for HugeTLB VMAs.
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.
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?
On 2020-01-20 at 17:51 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. IIUC the current >proposal will enable early bail out and prevent migration via migrate_pages() >at a much higher level like mbind() and other system calls if corresponding >VMA is HugeTLB but it's hstate lacks migration support. This should probably >save some cycles for HugeTLB VMAs. Thanks for comments. Yes，this patch is for enable early bail out.
On 2020-01-20 at 18:12 Michal Hocko wrote: >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. > Currently, missing check supported hugtlb size don't cause visible failure, because unmap_and_move_huge_page() will check it when try moving page. This part of changelog will be updated. >> 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. Yes, this is a code robustness cleanup, and for more accurate semantics. > >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. It seems no need to metion this part. 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. --- >-- >Michal Hocko >SUSE Labs
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.
On 01/20/2020 05:02 PM, Michal Hocko wrote: > 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. You are right that all hugetlb sizes are supported right now whether the platform defines arch_hugetlb_migration_supported() callback or not. But in theory an override for the arch callback can deny migration support for certain huge page sizes. > > 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? AFAICS this tries to solve the problem like a sub-optimal code. But for now as there are no real HugeTLB cases for an early bail out, there can be an argument not to add new cost into via vma_migratable() which will be called more often for non-HugeTLB VMAs. Probably adding a comment in the code like this might just be sufficient. diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h index 5228c62..ca9c343 100644 --- a/include/linux/mempolicy.h +++ b/include/linux/mempolicy.h @@ -186,6 +186,13 @@ static inline bool vma_migratable(struct vm_area_struct *vma) return false; #ifndef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION + /* + * NOTE: hugepage_migration_supported() should have been called here + * for an early bail out in cases where HugeTLB page sizes are not + * supported for migration. But for now as there are no such real + * cases, hence it is better not to add any additional cost here by + * calling hugepage_migration_supported(). + */ if (vma->vm_flags & VM_HUGETLB) return false; #endif
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. >
On 2020-01-21 at 00:05 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. > >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. Yes, this will be noted, thanks for emphersising this and it will help all to follow the change of code. >-- >Michal Hocko >SUSE Labs
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. >>
On 01/16/2020 09:41 AM, Li Xinhai wrote: > Checking hstate at early phase when isolating page, instead of during > unmap and move phase, to avoid useless isolation. > > Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Mike Kravetz <mike.kravetz@oracle.com> > --- Change log from the previous versions ? > include/linux/hugetlb.h | 10 ++++++++++ > include/linux/mempolicy.h | 29 +---------------------------- > mm/mempolicy.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 39 insertions(+), 28 deletions(-) > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index 31d4920..c9d871d 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -598,6 +598,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) > return arch_hugetlb_migration_supported(h); > } > > +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) > +{ > + return hugepage_migration_supported(hstate_vma(vma)); > +} Another wrapper around hugepage_migration_supported() is not necessary. > + > /* > * Movability check is different as compared to migration check. > * It determines whether or not a huge page should be placed on > @@ -809,6 +814,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) > return false; > } > > +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) > +{ > + return false; > +} > + > static inline bool hugepage_movable_supported(struct hstate *h) > { > return false; > diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h > index 5228c62..8165278 100644 > --- a/include/linux/mempolicy.h > +++ b/include/linux/mempolicy.h > @@ -173,34 +173,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from, > extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol); > > /* Check if a vma is migratable */ > -static inline bool vma_migratable(struct vm_area_struct *vma) > -{ > - if (vma->vm_flags & (VM_IO | VM_PFNMAP)) > - return false; > - > - /* > - * DAX device mappings require predictable access latency, so avoid > - * incurring periodic faults. > - */ > - if (vma_is_dax(vma)) > - return false; > - > -#ifndef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION > - if (vma->vm_flags & VM_HUGETLB) > - return false; > -#endif > - > - /* > - * Migration allocates pages in the highest zone. If we cannot > - * do so then migration (at least from node to node) is not > - * possible. > - */ > - if (vma->vm_file && > - gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) > - < policy_zone) > - return false; > - return true; > -} Why vma_migratable() is being moved ? > +extern bool vma_migratable(struct vm_area_struct *vma); > > extern int mpol_misplaced(struct page *, struct vm_area_struct *, unsigned long); > extern void mpol_put_task_policy(struct task_struct *); > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 067cf7d..8a01fb1 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1714,6 +1714,34 @@ static int kernel_get_mempolicy(int __user *policy, > > #endif /* CONFIG_COMPAT */ > > +bool vma_migratable(struct vm_area_struct *vma) > +{ > + if (vma->vm_flags & (VM_IO | VM_PFNMAP)) > + return false; > + > + /* > + * DAX device mappings require predictable access latency, so avoid > + * incurring periodic faults. > + */ > + if (vma_is_dax(vma)) > + return false; > + > + if (is_vm_hugetlb_page(vma) && > + !vm_hugepage_migration_supported(vma)) > + return false; This (use hugepage_migration_supported instead) can be added above without the code movement. > + > + /* > + * Migration allocates pages in the highest zone. If we cannot > + * do so then migration (at least from node to node) is not > + * possible. > + */ > + if (vma->vm_file && > + gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) > + < policy_zone) > + return false; > + return true; > +} > + > struct mempolicy *__get_vma_policy(struct vm_area_struct *vma, > unsigned long addr) > { >
On 2020-01-22 at 14:35 Anshuman Khandual wrote: > > >On 01/16/2020 09:41 AM, Li Xinhai wrote: >> Checking hstate at early phase when isolating page, instead of during >> unmap and move phase, to avoid useless isolation. >> >> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Mike Kravetz <mike.kravetz@oracle.com> >> --- > >Change log from the previous versions ? V2, V3 and V4 are all for using different ways to fix the circular reference of hugetlb.h and mempolicy.h. The exsiting relationship of these two files is allowing inline functions of hugetlb.h being able to refer symbols defined in mempolicy.h, but no feasible way for inline functions in mempolicy.h to using functions in hugetlb.h. After evaluated different fixes to this situation, current patch looks better , which no longer define vma_migratable as inline. Regarding to the new wrapper, yes it is not necessary. It is defined for checking at vma level, meant to provide different granularity for call from high level code(I noticed that in unmap_and_move_huge_page(), checking is done by hugepage_migration_supported(page_hstate(hpage)), and try using new wrapper which is different from that usage). If it looks too much redundant, change it OK. > >> include/linux/hugetlb.h | 10 ++++++++++ >> include/linux/mempolicy.h | 29 +---------------------------- >> mm/mempolicy.c | 28 ++++++++++++++++++++++++++++ >> 3 files changed, 39 insertions(+), 28 deletions(-) >> >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >> index 31d4920..c9d871d 100644 >> --- a/include/linux/hugetlb.h >> +++ b/include/linux/hugetlb.h >> @@ -598,6 +598,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) >> return arch_hugetlb_migration_supported(h); >> } >> >> +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) >> +{ >> + return hugepage_migration_supported(hstate_vma(vma)); >> +} > >Another wrapper around hugepage_migration_supported() is not necessary. Reason as above. > >> + >> /* >> * Movability check is different as compared to migration check. >> * It determines whether or not a huge page should be placed on >> @@ -809,6 +814,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) >> return false; >> } >> >> +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) >> +{ >> + return false; >> +} >> + >> static inline bool hugepage_movable_supported(struct hstate *h) >> { >> return false; >> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h >> index 5228c62..8165278 100644 >> --- a/include/linux/mempolicy.h >> +++ b/include/linux/mempolicy.h >> @@ -173,34 +173,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from, >> extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol); >> >> /* Check if a vma is migratable */ >> -static inline bool vma_migratable(struct vm_area_struct *vma) >> -{ >> - if (vma->vm_flags & (VM_IO | VM_PFNMAP)) >> - return false; >> - >> - /* >> - * DAX device mappings require predictable access latency, so avoid >> - * incurring periodic faults. >> - */ >> - if (vma_is_dax(vma)) >> - return false; >> - >> -#ifndef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION >> - if (vma->vm_flags & VM_HUGETLB) >> - return false; >> -#endif >> - >> - /* >> - * Migration allocates pages in the highest zone. If we cannot >> - * do so then migration (at least from node to node) is not >> - * possible. >> - */ >> - if (vma->vm_file && >> - gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) >> - < policy_zone) >> - return false; >> - return true; >> -} > >Why vma_migratable() is being moved ? Reason as above. > >> +extern bool vma_migratable(struct vm_area_struct *vma); >> >> extern int mpol_misplaced(struct page *, struct vm_area_struct *, unsigned long); >> extern void mpol_put_task_policy(struct task_struct *); >> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >> index 067cf7d..8a01fb1 100644 >> --- a/mm/mempolicy.c >> +++ b/mm/mempolicy.c >> @@ -1714,6 +1714,34 @@ static int kernel_get_mempolicy(int __user *policy, >> >> #endif /* CONFIG_COMPAT */ >> >> +bool vma_migratable(struct vm_area_struct *vma) >> +{ >> + if (vma->vm_flags & (VM_IO | VM_PFNMAP)) >> + return false; >> + >> + /* >> + * DAX device mappings require predictable access latency, so avoid >> + * incurring periodic faults. >> + */ >> + if (vma_is_dax(vma)) >> + return false; >> + >> + if (is_vm_hugetlb_page(vma) && >> + !vm_hugepage_migration_supported(vma)) >> + return false; > >This (use hugepage_migration_supported instead) can be added above without >the code movement. Reason as above. > >> + >> + /* >> + * Migration allocates pages in the highest zone. If we cannot >> + * do so then migration (at least from node to node) is not >> + * possible. >> + */ >> + if (vma->vm_file && >> + gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) >> + < policy_zone) >> + return false; >> + return true; >> +} >> + >> struct mempolicy *__get_vma_policy(struct vm_area_struct *vma, >> unsigned long addr) >> { >>
On 01/22/2020 06:51 PM, Li Xinhai wrote: > On 2020-01-22 at 14:35 Anshuman Khandual wrote: >> >> >> On 01/16/2020 09:41 AM, Li Xinhai wrote: >>> Checking hstate at early phase when isolating page, instead of during >>> unmap and move phase, to avoid useless isolation. >>> >>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> >>> Cc: Michal Hocko <mhocko@suse.com> >>> Cc: Mike Kravetz <mike.kravetz@oracle.com> >>> --- >> >> Change log from the previous versions ? > > V2, V3 and V4 are all for using different ways to fix the circular reference > of hugetlb.h and mempolicy.h. The exsiting relationship of these two files > is allowing inline functions of hugetlb.h being able to refer > symbols defined in mempolicy.h, but no feasible way for inline functions in > mempolicy.h to using functions in hugetlb.h. > After evaluated different fixes to this situation, current patch looks better > , which no longer define vma_migratable as inline. Okay but please always include the change log. > > Regarding to the new wrapper, yes it is not necessary. It is defined for > checking at vma level, meant to provide different granularity for call from > high level code(I noticed that in unmap_and_move_huge_page(), checking is done > by hugepage_migration_supported(page_hstate(hpage)), and try using new wrapper > which is different from that usage). If it looks too much redundant, change > it OK. Yeah its not really required as hstate can be easily derived from the VMA. Please drop this new wrapper. > >> >>> include/linux/hugetlb.h | 10 ++++++++++ >>> include/linux/mempolicy.h | 29 +---------------------------- >>> mm/mempolicy.c | 28 ++++++++++++++++++++++++++++ >>> 3 files changed, 39 insertions(+), 28 deletions(-) >>> >>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >>> index 31d4920..c9d871d 100644 >>> --- a/include/linux/hugetlb.h >>> +++ b/include/linux/hugetlb.h >>> @@ -598,6 +598,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) >>> return arch_hugetlb_migration_supported(h); >>> } >>> >>> +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) >>> +{ >>> + return hugepage_migration_supported(hstate_vma(vma)); >>> +} >> >> Another wrapper around hugepage_migration_supported() is not necessary. > > Reason as above. >> >>> + >>> /* >>> * Movability check is different as compared to migration check. >>> * It determines whether or not a huge page should be placed on >>> @@ -809,6 +814,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) >>> return false; >>> } >>> >>> +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) >>> +{ >>> + return false; >>> +} >>> + >>> static inline bool hugepage_movable_supported(struct hstate *h) >>> { >>> return false; >>> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h >>> index 5228c62..8165278 100644 >>> --- a/include/linux/mempolicy.h >>> +++ b/include/linux/mempolicy.h >>> @@ -173,34 +173,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from, >>> extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol); >>> >>> /* Check if a vma is migratable */ >>> -static inline bool vma_migratable(struct vm_area_struct *vma) >>> -{ >>> - if (vma->vm_flags & (VM_IO | VM_PFNMAP)) >>> - return false; >>> - >>> - /* >>> - * DAX device mappings require predictable access latency, so avoid >>> - * incurring periodic faults. >>> - */ >>> - if (vma_is_dax(vma)) >>> - return false; >>> - >>> -#ifndef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION >>> - if (vma->vm_flags & VM_HUGETLB) >>> - return false; >>> -#endif >>> - >>> - /* >>> - * Migration allocates pages in the highest zone. If we cannot >>> - * do so then migration (at least from node to node) is not >>> - * possible. >>> - */ >>> - if (vma->vm_file && >>> - gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) >>> - < policy_zone) >>> - return false; >>> - return true; >>> -} >> >> Why vma_migratable() is being moved ? > Reason as above. > >> >>> +extern bool vma_migratable(struct vm_area_struct *vma); >>> >>> extern int mpol_misplaced(struct page *, struct vm_area_struct *, unsigned long); >>> extern void mpol_put_task_policy(struct task_struct *); >>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >>> index 067cf7d..8a01fb1 100644 >>> --- a/mm/mempolicy.c >>> +++ b/mm/mempolicy.c >>> @@ -1714,6 +1714,34 @@ static int kernel_get_mempolicy(int __user *policy, >>> >>> #endif /* CONFIG_COMPAT */ >>> >>> +bool vma_migratable(struct vm_area_struct *vma) >>> +{ >>> + if (vma->vm_flags & (VM_IO | VM_PFNMAP)) >>> + return false; >>> + >>> + /* >>> + * DAX device mappings require predictable access latency, so avoid >>> + * incurring periodic faults. >>> + */ >>> + if (vma_is_dax(vma)) >>> + return false; >>> + >>> + if (is_vm_hugetlb_page(vma) && >>> + !vm_hugepage_migration_supported(vma)) >>> + return false; >> >> This (use hugepage_migration_supported instead) can be added above without >> the code movement. > Reason as above. >> >>> + >>> + /* >>> + * Migration allocates pages in the highest zone. If we cannot >>> + * do so then migration (at least from node to node) is not >>> + * possible. >>> + */ >>> + if (vma->vm_file && >>> + gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) >>> + < policy_zone) >>> + return false; >>> + return true; >>> +} >>> + >>> struct mempolicy *__get_vma_policy(struct vm_area_struct *vma, >>> unsigned long addr) >>> { >> >
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 31d4920..c9d871d 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -598,6 +598,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) return arch_hugetlb_migration_supported(h); } +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) +{ + return hugepage_migration_supported(hstate_vma(vma)); +} + /* * Movability check is different as compared to migration check. * It determines whether or not a huge page should be placed on @@ -809,6 +814,11 @@ static inline bool hugepage_migration_supported(struct hstate *h) return false; } +static inline bool vm_hugepage_migration_supported(struct vm_area_struct *vma) +{ + return false; +} + static inline bool hugepage_movable_supported(struct hstate *h) { return false; diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h index 5228c62..8165278 100644 --- a/include/linux/mempolicy.h +++ b/include/linux/mempolicy.h @@ -173,34 +173,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from, extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol); /* Check if a vma is migratable */ -static inline bool vma_migratable(struct vm_area_struct *vma) -{ - if (vma->vm_flags & (VM_IO | VM_PFNMAP)) - return false; - - /* - * DAX device mappings require predictable access latency, so avoid - * incurring periodic faults. - */ - if (vma_is_dax(vma)) - return false; - -#ifndef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION - if (vma->vm_flags & VM_HUGETLB) - return false; -#endif - - /* - * Migration allocates pages in the highest zone. If we cannot - * do so then migration (at least from node to node) is not - * possible. - */ - if (vma->vm_file && - gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) - < policy_zone) - return false; - return true; -} +extern bool vma_migratable(struct vm_area_struct *vma); extern int mpol_misplaced(struct page *, struct vm_area_struct *, unsigned long); extern void mpol_put_task_policy(struct task_struct *); diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 067cf7d..8a01fb1 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1714,6 +1714,34 @@ static int kernel_get_mempolicy(int __user *policy, #endif /* CONFIG_COMPAT */ +bool vma_migratable(struct vm_area_struct *vma) +{ + if (vma->vm_flags & (VM_IO | VM_PFNMAP)) + return false; + + /* + * DAX device mappings require predictable access latency, so avoid + * incurring periodic faults. + */ + if (vma_is_dax(vma)) + return false; + + if (is_vm_hugetlb_page(vma) && + !vm_hugepage_migration_supported(vma)) + return false; + + /* + * Migration allocates pages in the highest zone. If we cannot + * do so then migration (at least from node to node) is not + * possible. + */ + if (vma->vm_file && + gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) + < policy_zone) + return false; + return true; +} + struct mempolicy *__get_vma_policy(struct vm_area_struct *vma, unsigned long addr) {
Checking hstate at early phase when isolating page, instead of during unmap and move phase, to avoid useless isolation. Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com> Cc: Michal Hocko <mhocko@suse.com> Cc: Mike Kravetz <mike.kravetz@oracle.com> --- include/linux/hugetlb.h | 10 ++++++++++ include/linux/mempolicy.h | 29 +---------------------------- mm/mempolicy.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 28 deletions(-)