| Message ID | 20180622035151.6676-2-ying.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying <ying.huang@intel.com> wrote: > > From: Huang Ying <ying.huang@intel.com> > > Previously, the PMD swap operations are only enabled for > CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the > THP migration support. We will support PMD swap mapping to the huge > swap cluster and swapin the THP as a whole. That will be enabled via > CONFIG_THP_SWAP and needs these PMD swap operations. So enable the > PMD swap operations for CONFIG_THP_SWAP too. > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > Cc: Andrea Arcangeli <aarcange@redhat.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Shaohua Li <shli@kernel.org> > Cc: Hugh Dickins <hughd@google.com> > Cc: Minchan Kim <minchan@kernel.org> > Cc: Rik van Riel <riel@redhat.com> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> > Cc: Zi Yan <zi.yan@cs.rutgers.edu> > Cc: Daniel Jordan <daniel.m.jordan@oracle.com> > --- > arch/x86/include/asm/pgtable.h | 2 +- > include/asm-generic/pgtable.h | 2 +- > include/linux/swapops.h | 44 ++++++++++++++++++++++-------------------- > 3 files changed, 25 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h > index 99ecde23c3ec..13bf58838daf 100644 > --- a/arch/x86/include/asm/pgtable.h > +++ b/arch/x86/include/asm/pgtable.h > @@ -1224,7 +1224,7 @@ static inline pte_t pte_swp_clear_soft_dirty(pte_t pte) > return pte_clear_flags(pte, _PAGE_SWP_SOFT_DIRTY); > } > > -#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION > +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_THP_SWAP) How about introducing a new config symbol representing the common infrastructure between the two and have them select that symbol. Would that also allow us to clean up the usage of CONFIG_ARCH_ENABLE_THP_MIGRATION in fs/proc/task_mmu.c? In other words, what's the point of having nice ifdef'd alternatives in header files when ifdefs are still showing up in C files, all of it should be optionally determined by header files.
Dan Williams <dan.j.williams@intel.com> writes: > On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying <ying.huang@intel.com> wrote: >> >> From: Huang Ying <ying.huang@intel.com> >> >> Previously, the PMD swap operations are only enabled for >> CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the >> THP migration support. We will support PMD swap mapping to the huge >> swap cluster and swapin the THP as a whole. That will be enabled via >> CONFIG_THP_SWAP and needs these PMD swap operations. So enable the >> PMD swap operations for CONFIG_THP_SWAP too. >> >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> >> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> Cc: Andrea Arcangeli <aarcange@redhat.com> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Johannes Weiner <hannes@cmpxchg.org> >> Cc: Shaohua Li <shli@kernel.org> >> Cc: Hugh Dickins <hughd@google.com> >> Cc: Minchan Kim <minchan@kernel.org> >> Cc: Rik van Riel <riel@redhat.com> >> Cc: Dave Hansen <dave.hansen@linux.intel.com> >> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> >> Cc: Zi Yan <zi.yan@cs.rutgers.edu> >> Cc: Daniel Jordan <daniel.m.jordan@oracle.com> >> --- >> arch/x86/include/asm/pgtable.h | 2 +- >> include/asm-generic/pgtable.h | 2 +- >> include/linux/swapops.h | 44 ++++++++++++++++++++++-------------------- >> 3 files changed, 25 insertions(+), 23 deletions(-) >> >> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h >> index 99ecde23c3ec..13bf58838daf 100644 >> --- a/arch/x86/include/asm/pgtable.h >> +++ b/arch/x86/include/asm/pgtable.h >> @@ -1224,7 +1224,7 @@ static inline pte_t pte_swp_clear_soft_dirty(pte_t pte) >> return pte_clear_flags(pte, _PAGE_SWP_SOFT_DIRTY); >> } >> >> -#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION >> +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_THP_SWAP) > > How about introducing a new config symbol representing the common > infrastructure between the two and have them select that symbol. The common infrastructure shared by two mechanisms is PMD swap entry. But I didn't find there are many places where the common infrastructure is used. So I think it may be over-engineering to introduce a new config symbol but use it for so few times. > Would that also allow us to clean up the usage of > CONFIG_ARCH_ENABLE_THP_MIGRATION in fs/proc/task_mmu.c? In other > words, what's the point of having nice ifdef'd alternatives in header > files when ifdefs are still showing up in C files, all of it should be > optionally determined by header files. Unfortunately, I think it is not a easy task to wrap all C code via #ifdef in header files. And it may be over-engineering to wrap them all. I guess this is why there are still some #ifdef in C files. Best Regards, Huang, Ying
On Sun, Jul 8, 2018 at 10:40 PM, Huang, Ying <ying.huang@intel.com> wrote: > Dan Williams <dan.j.williams@intel.com> writes: > >> On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying <ying.huang@intel.com> wrote: >>> >>> From: Huang Ying <ying.huang@intel.com> >>> >>> Previously, the PMD swap operations are only enabled for >>> CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the >>> THP migration support. We will support PMD swap mapping to the huge >>> swap cluster and swapin the THP as a whole. That will be enabled via >>> CONFIG_THP_SWAP and needs these PMD swap operations. So enable the >>> PMD swap operations for CONFIG_THP_SWAP too. >>> >>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> >>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >>> Cc: Andrea Arcangeli <aarcange@redhat.com> >>> Cc: Michal Hocko <mhocko@suse.com> >>> Cc: Johannes Weiner <hannes@cmpxchg.org> >>> Cc: Shaohua Li <shli@kernel.org> >>> Cc: Hugh Dickins <hughd@google.com> >>> Cc: Minchan Kim <minchan@kernel.org> >>> Cc: Rik van Riel <riel@redhat.com> >>> Cc: Dave Hansen <dave.hansen@linux.intel.com> >>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> >>> Cc: Zi Yan <zi.yan@cs.rutgers.edu> >>> Cc: Daniel Jordan <daniel.m.jordan@oracle.com> >>> --- >>> arch/x86/include/asm/pgtable.h | 2 +- >>> include/asm-generic/pgtable.h | 2 +- >>> include/linux/swapops.h | 44 ++++++++++++++++++++++-------------------- >>> 3 files changed, 25 insertions(+), 23 deletions(-) >>> >>> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h >>> index 99ecde23c3ec..13bf58838daf 100644 >>> --- a/arch/x86/include/asm/pgtable.h >>> +++ b/arch/x86/include/asm/pgtable.h >>> @@ -1224,7 +1224,7 @@ static inline pte_t pte_swp_clear_soft_dirty(pte_t pte) >>> return pte_clear_flags(pte, _PAGE_SWP_SOFT_DIRTY); >>> } >>> >>> -#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION >>> +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_THP_SWAP) >> >> How about introducing a new config symbol representing the common >> infrastructure between the two and have them select that symbol. > > The common infrastructure shared by two mechanisms is PMD swap entry. > But I didn't find there are many places where the common infrastructure > is used. So I think it may be over-engineering to introduce a new > config symbol but use it for so few times. > >> Would that also allow us to clean up the usage of >> CONFIG_ARCH_ENABLE_THP_MIGRATION in fs/proc/task_mmu.c? In other >> words, what's the point of having nice ifdef'd alternatives in header >> files when ifdefs are still showing up in C files, all of it should be >> optionally determined by header files. > > Unfortunately, I think it is not a easy task to wrap all C code via > #ifdef in header files. And it may be over-engineering to wrap them > all. I guess this is why there are still some #ifdef in C files. That's the entire point. Yes, over-engineer the header files so the actual C code is more readable.
Dan Williams <dan.j.williams@intel.com> writes: > On Sun, Jul 8, 2018 at 10:40 PM, Huang, Ying <ying.huang@intel.com> wrote: >> Dan Williams <dan.j.williams@intel.com> writes: >>> Would that also allow us to clean up the usage of >>> CONFIG_ARCH_ENABLE_THP_MIGRATION in fs/proc/task_mmu.c? In other >>> words, what's the point of having nice ifdef'd alternatives in header >>> files when ifdefs are still showing up in C files, all of it should be >>> optionally determined by header files. >> >> Unfortunately, I think it is not a easy task to wrap all C code via >> #ifdef in header files. And it may be over-engineering to wrap them >> all. I guess this is why there are still some #ifdef in C files. > > That's the entire point. Yes, over-engineer the header files so the > actual C code is more readable. Take pagemap_pmd_range() in fs/proc/task_mmu.c as an example, to avoid #ifdef, we may wrap all code between #ifdef/#endif into a separate function, put the new function into another C file (which is compiled only if #ifdef is true), then change header files for that too. In this way, we avoid #ifdef/#endif, but the code is more complex and tightly related code may be put into different files. The readability may be hurt too. Maybe you have smarter way to change the code to avoid #ifdef and improve code readability? Best Regards, Huang, Ying
On 06/21/2018 08:51 PM, Huang, Ying wrote: > From: Huang Ying <ying.huang@intel.com> > > Previously, the PMD swap operations are only enabled for > CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the > THP migration support. We will support PMD swap mapping to the huge > swap cluster and swapin the THP as a whole. That will be enabled via > CONFIG_THP_SWAP and needs these PMD swap operations. So enable the > PMD swap operations for CONFIG_THP_SWAP too. This commit message kinda skirts around the real reasons for this patch. Shouldn't we just say something like: Currently, "swap entries" in the page tables are used for a number of things outside of actual swap, like page migration. We support THP/PMD "swap entries" for page migration currently and the functions behind this are tied to page migration's config option (CONFIG_ARCH_ENABLE_THP_MIGRATION). But, we also need them for THP swap. ... It would also be nice to explain a bit why you are moving code around. Would this look any better if we made a Kconfig option: config HAVE_THP_SWAP_ENTRIES def_bool n # "Swap entries" in the page tables are used # both for migration and actual swap. depends on THP_SWAP || ARCH_ENABLE_THP_MIGRATION You logically talked about this need for PMD swap operations in your commit message, so I think it makes sense to codify that in a single place where it can be coherently explained.
Dave Hansen <dave.hansen@linux.intel.com> writes: > On 06/21/2018 08:51 PM, Huang, Ying wrote: >> From: Huang Ying <ying.huang@intel.com> >> >> Previously, the PMD swap operations are only enabled for >> CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the >> THP migration support. We will support PMD swap mapping to the huge >> swap cluster and swapin the THP as a whole. That will be enabled via >> CONFIG_THP_SWAP and needs these PMD swap operations. So enable the >> PMD swap operations for CONFIG_THP_SWAP too. > > This commit message kinda skirts around the real reasons for this patch. > Shouldn't we just say something like: > > Currently, "swap entries" in the page tables are used for a > number of things outside of actual swap, like page migration. > We support THP/PMD "swap entries" for page migration currently > and the functions behind this are tied to page migration's > config option (CONFIG_ARCH_ENABLE_THP_MIGRATION). > > But, we also need them for THP swap. > ... > > It would also be nice to explain a bit why you are moving code around. This looks much better than my original words. Thanks for help! > Would this look any better if we made a Kconfig option: > > config HAVE_THP_SWAP_ENTRIES > def_bool n > # "Swap entries" in the page tables are used > # both for migration and actual swap. > depends on THP_SWAP || ARCH_ENABLE_THP_MIGRATION > > You logically talked about this need for PMD swap operations in your > commit message, so I think it makes sense to codify that in a single > place where it can be coherently explained. Because both you and Dan thinks it's better to add new Kconfig option, I will do that. Best Regards, Huang, Ying
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index 99ecde23c3ec..13bf58838daf 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -1224,7 +1224,7 @@ static inline pte_t pte_swp_clear_soft_dirty(pte_t pte) return pte_clear_flags(pte, _PAGE_SWP_SOFT_DIRTY); } -#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_THP_SWAP) static inline pmd_t pmd_swp_mksoft_dirty(pmd_t pmd) { return pmd_set_flags(pmd, _PAGE_SWP_SOFT_DIRTY); diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h index f59639afaa39..bb8354981a36 100644 --- a/include/asm-generic/pgtable.h +++ b/include/asm-generic/pgtable.h @@ -675,7 +675,7 @@ static inline void ptep_modify_prot_commit(struct mm_struct *mm, #endif #ifdef CONFIG_HAVE_ARCH_SOFT_DIRTY -#ifndef CONFIG_ARCH_ENABLE_THP_MIGRATION +#if !defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) && !defined(CONFIG_THP_SWAP) static inline pmd_t pmd_swp_mksoft_dirty(pmd_t pmd) { return pmd; diff --git a/include/linux/swapops.h b/include/linux/swapops.h index 1d3877c39a00..f1be5a52f5c8 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -258,17 +258,7 @@ static inline int is_write_migration_entry(swp_entry_t entry) #endif -struct page_vma_mapped_walk; - -#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION -extern void set_pmd_migration_entry(struct page_vma_mapped_walk *pvmw, - struct page *page); - -extern void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, - struct page *new); - -extern void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd); - +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_THP_SWAP) static inline swp_entry_t pmd_to_swp_entry(pmd_t pmd) { swp_entry_t arch_entry; @@ -286,6 +276,28 @@ static inline pmd_t swp_entry_to_pmd(swp_entry_t entry) arch_entry = __swp_entry(swp_type(entry), swp_offset(entry)); return __swp_entry_to_pmd(arch_entry); } +#else +static inline swp_entry_t pmd_to_swp_entry(pmd_t pmd) +{ + return swp_entry(0, 0); +} + +static inline pmd_t swp_entry_to_pmd(swp_entry_t entry) +{ + return __pmd(0); +} +#endif + +struct page_vma_mapped_walk; + +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION +extern void set_pmd_migration_entry(struct page_vma_mapped_walk *pvmw, + struct page *page); + +extern void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, + struct page *new); + +extern void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd); static inline int is_pmd_migration_entry(pmd_t pmd) { @@ -306,16 +318,6 @@ static inline void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, static inline void pmd_migration_entry_wait(struct mm_struct *m, pmd_t *p) { } -static inline swp_entry_t pmd_to_swp_entry(pmd_t pmd) -{ - return swp_entry(0, 0); -} - -static inline pmd_t swp_entry_to_pmd(swp_entry_t entry) -{ - return __pmd(0); -} - static inline int is_pmd_migration_entry(pmd_t pmd) { return 0;