* [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition @ 2022-09-03 2:12 Zach O'Keefe 2022-09-06 18:46 ` Yang Shi 0 siblings, 1 reply; 7+ messages in thread From: Zach O'Keefe @ 2022-09-03 2:12 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-mm, Yang Shi, kbuild-all, Zach O'Keefe HPAGE_PMD_ORDER is used to determine a minimal amount of bits needed to store per-node counters in struct collapse_control, defining the array to be either a u16[HPAGE_PMD_ORDER] or u32[HPAGE_PMD_ORDER]. For some architectures/configs, HPAGE_PMD_ORDER isn't resolved at preprocessor time (suitable for resolving preprocessor directives) but rather expands into a compiler built-in that is resolved at compile-time. E.g. for 32-bit MIPS (see link for full config): ---8<-- >> arch/mips/include/asm/pgtable.h:238:26: warning: "__builtin_ffs" is not defined, evaluates to 0 [-Wundef] 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) | ^~~~~~~~~~~~~ arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) | ^~~~~~~~~~ include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' 11 | #define P4D_SHIFT PGDIR_SHIFT | ^~~~~~~~~~~ include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' 18 | #define PUD_SHIFT P4D_SHIFT | ^~~~~~~~~ include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' 20 | #define PMD_SHIFT PUD_SHIFT | ^~~~~~~~~ include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT | ^~~~~~~~~ include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) | ^~~~~~~~~~~~~~~ mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' 90 | #if HPAGE_PMD_ORDER < 16 | ^~~~~~~~~~~~~~~ >> arch/mips/include/asm/pgtable.h:238:39: error: missing binary operator before token "(" 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) | ^ arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) | ^~~~~~~~~~ include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' 11 | #define P4D_SHIFT PGDIR_SHIFT | ^~~~~~~~~~~ include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' 18 | #define PUD_SHIFT P4D_SHIFT | ^~~~~~~~~ include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' 20 | #define PMD_SHIFT PUD_SHIFT | ^~~~~~~~~ include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT | ^~~~~~~~~ include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) | ^~~~~~~~~~~~~~~ mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' 90 | #if HPAGE_PMD_ORDER < 16 | ^~~~~~~~~~~~~~~ ---8<-- load_node is already special-cased for CONFIG_PPC64 in "mm-khugepaged-add-struct-collapse_control-fix-fix", and we could continue down that path by #define'ing away edge cases, but I think at this point it easier to just let load_node be an array of u32s. If it's ever an issue, we can revisit. This essentially rolls back both: "mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR" "mm-khugepaged-add-struct-collapse_control-fix-fix" (though we'll leave it as a u32 vs an int). Link: https://lore.kernel.org/linux-mm/202209021349.F73i5d6X-lkp@intel.com/ Fixes: 5f7bb00bd9cd ("mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR") Fixes: 03053c821af2 ("mm-khugepaged-add-struct-collapse_control-fix-fix") Signed-off-by: Zach O'Keefe <zokeefe@google.com> --- mm/khugepaged.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/mm/khugepaged.c b/mm/khugepaged.c index af3b07eb2389..29142a35d24c 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -93,13 +93,8 @@ struct collapse_control { bool is_khugepaged; /* Num pages scanned per node */ -#if defined(CONFIG_PPC64) u32 node_load[MAX_NUMNODES]; -#elif HPAGE_PMD_ORDER < 16 - u16 node_load[MAX_NUMNODES]; -#else - u32 node_load[MAX_NUMNODES]; -#endif + /* Last target selected in hpage_collapse_find_target_node() */ int last_target_node; }; -- 2.37.2.789.g6183377224-goog ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition 2022-09-03 2:12 [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition Zach O'Keefe @ 2022-09-06 18:46 ` Yang Shi 2022-09-06 19:13 ` Zach O'Keefe 0 siblings, 1 reply; 7+ messages in thread From: Yang Shi @ 2022-09-06 18:46 UTC (permalink / raw) To: Zach O'Keefe; +Cc: Andrew Morton, linux-mm, kbuild-all On Fri, Sep 2, 2022 at 7:12 PM Zach O'Keefe <zokeefe@google.com> wrote: > > HPAGE_PMD_ORDER is used to determine a minimal amount of bits needed to store > per-node counters in struct collapse_control, defining the array to be either a > u16[HPAGE_PMD_ORDER] or u32[HPAGE_PMD_ORDER]. > > For some architectures/configs, HPAGE_PMD_ORDER isn't resolved at preprocessor > time (suitable for resolving preprocessor directives) but rather expands into a > compiler built-in that is resolved at compile-time. E.g. for 32-bit MIPS (see > link for full config): > > ---8<-- > >> arch/mips/include/asm/pgtable.h:238:26: warning: "__builtin_ffs" is not defined, evaluates to 0 [-Wundef] > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > | ^~~~~~~~~~~~~ > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > | ^~~~~~~~~~ > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > 11 | #define P4D_SHIFT PGDIR_SHIFT > | ^~~~~~~~~~~ > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > 18 | #define PUD_SHIFT P4D_SHIFT > | ^~~~~~~~~ > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > 20 | #define PMD_SHIFT PUD_SHIFT > | ^~~~~~~~~ > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > | ^~~~~~~~~ > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > | ^~~~~~~~~~~~~~~ > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > 90 | #if HPAGE_PMD_ORDER < 16 > | ^~~~~~~~~~~~~~~ > >> arch/mips/include/asm/pgtable.h:238:39: error: missing binary operator before token "(" > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > | ^ > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > | ^~~~~~~~~~ > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > 11 | #define P4D_SHIFT PGDIR_SHIFT > | ^~~~~~~~~~~ > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > 18 | #define PUD_SHIFT P4D_SHIFT > | ^~~~~~~~~ > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > 20 | #define PMD_SHIFT PUD_SHIFT > | ^~~~~~~~~ > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > | ^~~~~~~~~ > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > | ^~~~~~~~~~~~~~~ > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > 90 | #if HPAGE_PMD_ORDER < 16 > | ^~~~~~~~~~~~~~~ > ---8<-- > > load_node is already special-cased for CONFIG_PPC64 in > "mm-khugepaged-add-struct-collapse_control-fix-fix", and we could continue down > that path by #define'ing away edge cases, but I think at this point it easier > to just let load_node be an array of u32s. If it's ever an issue, we can > revisit. > > This essentially rolls back both: > > "mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR" > "mm-khugepaged-add-struct-collapse_control-fix-fix" > > (though we'll leave it as a u32 vs an int). > > Link: https://lore.kernel.org/linux-mm/202209021349.F73i5d6X-lkp@intel.com/ > Fixes: 5f7bb00bd9cd ("mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR") > Fixes: 03053c821af2 ("mm-khugepaged-add-struct-collapse_control-fix-fix") > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > --- > mm/khugepaged.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index af3b07eb2389..29142a35d24c 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -93,13 +93,8 @@ struct collapse_control { > bool is_khugepaged; > > /* Num pages scanned per node */ > -#if defined(CONFIG_PPC64) > u32 node_load[MAX_NUMNODES]; > -#elif HPAGE_PMD_ORDER < 16 > - u16 node_load[MAX_NUMNODES]; > -#else > - u32 node_load[MAX_NUMNODES]; > -#endif I recall we need this to circumvent some build time warning about stack usage? Or I misremember? > + > /* Last target selected in hpage_collapse_find_target_node() */ > int last_target_node; > }; > -- > 2.37.2.789.g6183377224-goog > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition 2022-09-06 18:46 ` Yang Shi @ 2022-09-06 19:13 ` Zach O'Keefe 2022-09-06 20:04 ` Yang Shi 0 siblings, 1 reply; 7+ messages in thread From: Zach O'Keefe @ 2022-09-06 19:13 UTC (permalink / raw) To: Yang Shi; +Cc: Andrew Morton, linux-mm, kbuild-all On Tue, Sep 6, 2022 at 11:46 AM Yang Shi <shy828301@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 7:12 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > HPAGE_PMD_ORDER is used to determine a minimal amount of bits needed to store > > per-node counters in struct collapse_control, defining the array to be either a > > u16[HPAGE_PMD_ORDER] or u32[HPAGE_PMD_ORDER]. > > > > For some architectures/configs, HPAGE_PMD_ORDER isn't resolved at preprocessor > > time (suitable for resolving preprocessor directives) but rather expands into a > > compiler built-in that is resolved at compile-time. E.g. for 32-bit MIPS (see > > link for full config): > > > > ---8<-- > > >> arch/mips/include/asm/pgtable.h:238:26: warning: "__builtin_ffs" is not defined, evaluates to 0 [-Wundef] > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > | ^~~~~~~~~~~~~ > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > | ^~~~~~~~~~ > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > | ^~~~~~~~~~~ > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > 18 | #define PUD_SHIFT P4D_SHIFT > > | ^~~~~~~~~ > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > 20 | #define PMD_SHIFT PUD_SHIFT > > | ^~~~~~~~~ > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > | ^~~~~~~~~ > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > | ^~~~~~~~~~~~~~~ > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > 90 | #if HPAGE_PMD_ORDER < 16 > > | ^~~~~~~~~~~~~~~ > > >> arch/mips/include/asm/pgtable.h:238:39: error: missing binary operator before token "(" > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > | ^ > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > | ^~~~~~~~~~ > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > | ^~~~~~~~~~~ > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > 18 | #define PUD_SHIFT P4D_SHIFT > > | ^~~~~~~~~ > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > 20 | #define PMD_SHIFT PUD_SHIFT > > | ^~~~~~~~~ > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > | ^~~~~~~~~ > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > | ^~~~~~~~~~~~~~~ > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > 90 | #if HPAGE_PMD_ORDER < 16 > > | ^~~~~~~~~~~~~~~ > > ---8<-- > > > > load_node is already special-cased for CONFIG_PPC64 in > > "mm-khugepaged-add-struct-collapse_control-fix-fix", and we could continue down > > that path by #define'ing away edge cases, but I think at this point it easier > > to just let load_node be an array of u32s. If it's ever an issue, we can > > revisit. > > > > This essentially rolls back both: > > > > "mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR" > > "mm-khugepaged-add-struct-collapse_control-fix-fix" > > > > (though we'll leave it as a u32 vs an int). > > > > Link: https://lore.kernel.org/linux-mm/202209021349.F73i5d6X-lkp@intel.com/ > > Fixes: 5f7bb00bd9cd ("mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR") > > Fixes: 03053c821af2 ("mm-khugepaged-add-struct-collapse_control-fix-fix") > > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > > --- > > mm/khugepaged.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index af3b07eb2389..29142a35d24c 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -93,13 +93,8 @@ struct collapse_control { > > bool is_khugepaged; > > > > /* Num pages scanned per node */ > > -#if defined(CONFIG_PPC64) > > u32 node_load[MAX_NUMNODES]; > > -#elif HPAGE_PMD_ORDER < 16 > > - u16 node_load[MAX_NUMNODES]; > > -#else > > - u32 node_load[MAX_NUMNODES]; > > -#endif > > I recall we need this to circumvent some build time warning about > stack usage? Or I misremember? > Hey Yang, thanks for keeping a look out here, and good memory! The stack usage issue was due to struct collapse_control being allocated on the stack when MAX_NUMNODES was massive[1]. Now that we kmalloc() them for MADV_COLLAPSE, idea is we'd want to limit required memory and the original change here was an optimization due to an observation that we really didn't need 32 bits to hold a maximum value of HPAGE_PMD_NR[2], but determining some safe upper bound for the number of requires bits per arch/config as been a bit of a pain, with special casing adding to support POWER64 already[3], and now this mips issue that would otherwise require something special. [1] https://lore.kernel.org/linux-mm/CAAa6QmQXn9odA-dsm-FFKqQmDvBuH0qDkmPFfXDS1so+Loe+4Q@mail.gmail.com/ [2] https://lore.kernel.org/linux-mm/Ys2CeIm%2FQmQwWh9a@google.com/ [3] https://lore.kernel.org/linux-next/Ytk0TQO9khUQgjxT@google.com/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition 2022-09-06 19:13 ` Zach O'Keefe @ 2022-09-06 20:04 ` Yang Shi 2022-09-06 20:27 ` Zach O'Keefe 0 siblings, 1 reply; 7+ messages in thread From: Yang Shi @ 2022-09-06 20:04 UTC (permalink / raw) To: Zach O'Keefe; +Cc: Andrew Morton, linux-mm, kbuild-all On Tue, Sep 6, 2022 at 12:14 PM Zach O'Keefe <zokeefe@google.com> wrote: > > On Tue, Sep 6, 2022 at 11:46 AM Yang Shi <shy828301@gmail.com> wrote: > > > > On Fri, Sep 2, 2022 at 7:12 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > > > HPAGE_PMD_ORDER is used to determine a minimal amount of bits needed to store > > > per-node counters in struct collapse_control, defining the array to be either a > > > u16[HPAGE_PMD_ORDER] or u32[HPAGE_PMD_ORDER]. > > > > > > For some architectures/configs, HPAGE_PMD_ORDER isn't resolved at preprocessor > > > time (suitable for resolving preprocessor directives) but rather expands into a > > > compiler built-in that is resolved at compile-time. E.g. for 32-bit MIPS (see > > > link for full config): > > > > > > ---8<-- > > > >> arch/mips/include/asm/pgtable.h:238:26: warning: "__builtin_ffs" is not defined, evaluates to 0 [-Wundef] > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > | ^~~~~~~~~~~~~ > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > | ^~~~~~~~~~ > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > | ^~~~~~~~~~~ > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > | ^~~~~~~~~ > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > | ^~~~~~~~~ > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > | ^~~~~~~~~ > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > | ^~~~~~~~~~~~~~~ > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > | ^~~~~~~~~~~~~~~ > > > >> arch/mips/include/asm/pgtable.h:238:39: error: missing binary operator before token "(" > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > | ^ > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > | ^~~~~~~~~~ > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > | ^~~~~~~~~~~ > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > | ^~~~~~~~~ > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > | ^~~~~~~~~ > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > | ^~~~~~~~~ > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > | ^~~~~~~~~~~~~~~ > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > | ^~~~~~~~~~~~~~~ > > > ---8<-- > > > > > > load_node is already special-cased for CONFIG_PPC64 in > > > "mm-khugepaged-add-struct-collapse_control-fix-fix", and we could continue down > > > that path by #define'ing away edge cases, but I think at this point it easier > > > to just let load_node be an array of u32s. If it's ever an issue, we can > > > revisit. > > > > > > This essentially rolls back both: > > > > > > "mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR" > > > "mm-khugepaged-add-struct-collapse_control-fix-fix" > > > > > > (though we'll leave it as a u32 vs an int). > > > > > > Link: https://lore.kernel.org/linux-mm/202209021349.F73i5d6X-lkp@intel.com/ > > > Fixes: 5f7bb00bd9cd ("mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR") > > > Fixes: 03053c821af2 ("mm-khugepaged-add-struct-collapse_control-fix-fix") > > > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > > > --- > > > mm/khugepaged.c | 7 +------ > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > > index af3b07eb2389..29142a35d24c 100644 > > > --- a/mm/khugepaged.c > > > +++ b/mm/khugepaged.c > > > @@ -93,13 +93,8 @@ struct collapse_control { > > > bool is_khugepaged; > > > > > > /* Num pages scanned per node */ > > > -#if defined(CONFIG_PPC64) > > > u32 node_load[MAX_NUMNODES]; > > > -#elif HPAGE_PMD_ORDER < 16 > > > - u16 node_load[MAX_NUMNODES]; > > > -#else > > > - u32 node_load[MAX_NUMNODES]; > > > -#endif > > > > I recall we need this to circumvent some build time warning about > > stack usage? Or I misremember? > > > > Hey Yang, thanks for keeping a look out here, and good memory! > > The stack usage issue was due to struct collapse_control being > allocated on the stack when MAX_NUMNODES was massive[1]. Now that we > kmalloc() them for MADV_COLLAPSE, idea is we'd want to limit required > memory and the original change here was an optimization due to an > observation that we really didn't need 32 bits to hold a maximum value > of HPAGE_PMD_NR[2], but determining some safe upper bound for the > number of requires bits per arch/config as been a bit of a pain, with > special casing adding to support POWER64 already[3], and now this mips > issue that would otherwise require something special. Aha, thanks for refreshing my memory. The patch is fine to me. I'm wondering whether it is worth introducing a new macro, for example, ARCH_HAVE_HPAGE_PMD_ORDER_MACRO (maybe not a good name), then we could make the code #ifdef the new macro. > > [1] https://lore.kernel.org/linux-mm/CAAa6QmQXn9odA-dsm-FFKqQmDvBuH0qDkmPFfXDS1so+Loe+4Q@mail.gmail.com/ > [2] https://lore.kernel.org/linux-mm/Ys2CeIm%2FQmQwWh9a@google.com/ > [3] https://lore.kernel.org/linux-next/Ytk0TQO9khUQgjxT@google.com/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition 2022-09-06 20:04 ` Yang Shi @ 2022-09-06 20:27 ` Zach O'Keefe 2022-09-07 17:07 ` Yang Shi 0 siblings, 1 reply; 7+ messages in thread From: Zach O'Keefe @ 2022-09-06 20:27 UTC (permalink / raw) To: Yang Shi; +Cc: Andrew Morton, linux-mm, kbuild-all On Tue, Sep 6, 2022 at 1:04 PM Yang Shi <shy828301@gmail.com> wrote: > > On Tue, Sep 6, 2022 at 12:14 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > On Tue, Sep 6, 2022 at 11:46 AM Yang Shi <shy828301@gmail.com> wrote: > > > > > > On Fri, Sep 2, 2022 at 7:12 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > > > > > HPAGE_PMD_ORDER is used to determine a minimal amount of bits needed to store > > > > per-node counters in struct collapse_control, defining the array to be either a > > > > u16[HPAGE_PMD_ORDER] or u32[HPAGE_PMD_ORDER]. > > > > > > > > For some architectures/configs, HPAGE_PMD_ORDER isn't resolved at preprocessor > > > > time (suitable for resolving preprocessor directives) but rather expands into a > > > > compiler built-in that is resolved at compile-time. E.g. for 32-bit MIPS (see > > > > link for full config): > > > > > > > > ---8<-- > > > > >> arch/mips/include/asm/pgtable.h:238:26: warning: "__builtin_ffs" is not defined, evaluates to 0 [-Wundef] > > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > > | ^~~~~~~~~~~~~ > > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > > | ^~~~~~~~~~ > > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > > | ^~~~~~~~~~~ > > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > > | ^~~~~~~~~ > > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > > | ^~~~~~~~~ > > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > > | ^~~~~~~~~ > > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > > | ^~~~~~~~~~~~~~~ > > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > > | ^~~~~~~~~~~~~~~ > > > > >> arch/mips/include/asm/pgtable.h:238:39: error: missing binary operator before token "(" > > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > > | ^ > > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > > | ^~~~~~~~~~ > > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > > | ^~~~~~~~~~~ > > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > > | ^~~~~~~~~ > > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > > | ^~~~~~~~~ > > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > > | ^~~~~~~~~ > > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > > | ^~~~~~~~~~~~~~~ > > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > > | ^~~~~~~~~~~~~~~ > > > > ---8<-- > > > > > > > > load_node is already special-cased for CONFIG_PPC64 in > > > > "mm-khugepaged-add-struct-collapse_control-fix-fix", and we could continue down > > > > that path by #define'ing away edge cases, but I think at this point it easier > > > > to just let load_node be an array of u32s. If it's ever an issue, we can > > > > revisit. > > > > > > > > This essentially rolls back both: > > > > > > > > "mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR" > > > > "mm-khugepaged-add-struct-collapse_control-fix-fix" > > > > > > > > (though we'll leave it as a u32 vs an int). > > > > > > > > Link: https://lore.kernel.org/linux-mm/202209021349.F73i5d6X-lkp@intel.com/ > > > > Fixes: 5f7bb00bd9cd ("mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR") > > > > Fixes: 03053c821af2 ("mm-khugepaged-add-struct-collapse_control-fix-fix") > > > > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > > > > --- > > > > mm/khugepaged.c | 7 +------ > > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > > > index af3b07eb2389..29142a35d24c 100644 > > > > --- a/mm/khugepaged.c > > > > +++ b/mm/khugepaged.c > > > > @@ -93,13 +93,8 @@ struct collapse_control { > > > > bool is_khugepaged; > > > > > > > > /* Num pages scanned per node */ > > > > -#if defined(CONFIG_PPC64) > > > > u32 node_load[MAX_NUMNODES]; > > > > -#elif HPAGE_PMD_ORDER < 16 > > > > - u16 node_load[MAX_NUMNODES]; > > > > -#else > > > > - u32 node_load[MAX_NUMNODES]; > > > > -#endif > > > > > > I recall we need this to circumvent some build time warning about > > > stack usage? Or I misremember? > > > > > > > Hey Yang, thanks for keeping a look out here, and good memory! > > > > The stack usage issue was due to struct collapse_control being > > allocated on the stack when MAX_NUMNODES was massive[1]. Now that we > > kmalloc() them for MADV_COLLAPSE, idea is we'd want to limit required > > memory and the original change here was an optimization due to an > > observation that we really didn't need 32 bits to hold a maximum value > > of HPAGE_PMD_NR[2], but determining some safe upper bound for the > > number of requires bits per arch/config as been a bit of a pain, with > > special casing adding to support POWER64 already[3], and now this mips > > issue that would otherwise require something special. > > Aha, thanks for refreshing my memory. > > The patch is fine to me. I'm wondering whether it is worth introducing > a new macro, for example, ARCH_HAVE_HPAGE_PMD_ORDER_MACRO (maybe not a > good name), then we could make the code #ifdef the new macro. Likewise, I don't know if it's worth it or not. The config generating the original -Wframe-larger-than= had 1024 nodes. If we use that as an upper bound the number of nodes in a system then it's a difference of 2 KiB / MADV_COLLAPSE call in the extreme case. Now, this is the first time I've actually used kmalloc(), so I don't have a good sense of how frugal we should be with memory. 2 KiB does sound like a lot - but I suspect the common case will be < 32 bytes. Do we have a sense for the largest sane CONFIG_NODES_SHIFT? The only systems I deal with are 1,2 or 4 nodes, so I don't know what a sensible upper bound is here. > > > > > [1] https://lore.kernel.org/linux-mm/CAAa6QmQXn9odA-dsm-FFKqQmDvBuH0qDkmPFfXDS1so+Loe+4Q@mail.gmail.com/ > > [2] https://lore.kernel.org/linux-mm/Ys2CeIm%2FQmQwWh9a@google.com/ > > [3] https://lore.kernel.org/linux-next/Ytk0TQO9khUQgjxT@google.com/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition 2022-09-06 20:27 ` Zach O'Keefe @ 2022-09-07 17:07 ` Yang Shi 2022-09-07 18:47 ` Zach O'Keefe 0 siblings, 1 reply; 7+ messages in thread From: Yang Shi @ 2022-09-07 17:07 UTC (permalink / raw) To: Zach O'Keefe; +Cc: Andrew Morton, linux-mm, kbuild-all On Tue, Sep 6, 2022 at 1:28 PM Zach O'Keefe <zokeefe@google.com> wrote: > > On Tue, Sep 6, 2022 at 1:04 PM Yang Shi <shy828301@gmail.com> wrote: > > > > On Tue, Sep 6, 2022 at 12:14 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > > > On Tue, Sep 6, 2022 at 11:46 AM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > On Fri, Sep 2, 2022 at 7:12 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > > > > > > > HPAGE_PMD_ORDER is used to determine a minimal amount of bits needed to store > > > > > per-node counters in struct collapse_control, defining the array to be either a > > > > > u16[HPAGE_PMD_ORDER] or u32[HPAGE_PMD_ORDER]. > > > > > > > > > > For some architectures/configs, HPAGE_PMD_ORDER isn't resolved at preprocessor > > > > > time (suitable for resolving preprocessor directives) but rather expands into a > > > > > compiler built-in that is resolved at compile-time. E.g. for 32-bit MIPS (see > > > > > link for full config): > > > > > > > > > > ---8<-- > > > > > >> arch/mips/include/asm/pgtable.h:238:26: warning: "__builtin_ffs" is not defined, evaluates to 0 [-Wundef] > > > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > > > | ^~~~~~~~~~~~~ > > > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > > > | ^~~~~~~~~~ > > > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > > > | ^~~~~~~~~~~ > > > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > > > | ^~~~~~~~~ > > > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > > > | ^~~~~~~~~ > > > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > > > | ^~~~~~~~~ > > > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > > > | ^~~~~~~~~~~~~~~ > > > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > > > | ^~~~~~~~~~~~~~~ > > > > > >> arch/mips/include/asm/pgtable.h:238:39: error: missing binary operator before token "(" > > > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > > > | ^ > > > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > > > | ^~~~~~~~~~ > > > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > > > | ^~~~~~~~~~~ > > > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > > > | ^~~~~~~~~ > > > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > > > | ^~~~~~~~~ > > > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > > > | ^~~~~~~~~ > > > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > > > | ^~~~~~~~~~~~~~~ > > > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > > > | ^~~~~~~~~~~~~~~ > > > > > ---8<-- > > > > > > > > > > load_node is already special-cased for CONFIG_PPC64 in > > > > > "mm-khugepaged-add-struct-collapse_control-fix-fix", and we could continue down > > > > > that path by #define'ing away edge cases, but I think at this point it easier > > > > > to just let load_node be an array of u32s. If it's ever an issue, we can > > > > > revisit. > > > > > > > > > > This essentially rolls back both: > > > > > > > > > > "mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR" > > > > > "mm-khugepaged-add-struct-collapse_control-fix-fix" > > > > > > > > > > (though we'll leave it as a u32 vs an int). > > > > > > > > > > Link: https://lore.kernel.org/linux-mm/202209021349.F73i5d6X-lkp@intel.com/ > > > > > Fixes: 5f7bb00bd9cd ("mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR") > > > > > Fixes: 03053c821af2 ("mm-khugepaged-add-struct-collapse_control-fix-fix") > > > > > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > > > > > --- > > > > > mm/khugepaged.c | 7 +------ > > > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > > > > index af3b07eb2389..29142a35d24c 100644 > > > > > --- a/mm/khugepaged.c > > > > > +++ b/mm/khugepaged.c > > > > > @@ -93,13 +93,8 @@ struct collapse_control { > > > > > bool is_khugepaged; > > > > > > > > > > /* Num pages scanned per node */ > > > > > -#if defined(CONFIG_PPC64) > > > > > u32 node_load[MAX_NUMNODES]; > > > > > -#elif HPAGE_PMD_ORDER < 16 > > > > > - u16 node_load[MAX_NUMNODES]; > > > > > -#else > > > > > - u32 node_load[MAX_NUMNODES]; > > > > > -#endif > > > > > > > > I recall we need this to circumvent some build time warning about > > > > stack usage? Or I misremember? > > > > > > > > > > Hey Yang, thanks for keeping a look out here, and good memory! > > > > > > The stack usage issue was due to struct collapse_control being > > > allocated on the stack when MAX_NUMNODES was massive[1]. Now that we > > > kmalloc() them for MADV_COLLAPSE, idea is we'd want to limit required > > > memory and the original change here was an optimization due to an > > > observation that we really didn't need 32 bits to hold a maximum value > > > of HPAGE_PMD_NR[2], but determining some safe upper bound for the > > > number of requires bits per arch/config as been a bit of a pain, with > > > special casing adding to support POWER64 already[3], and now this mips > > > issue that would otherwise require something special. > > > > Aha, thanks for refreshing my memory. > > > > The patch is fine to me. I'm wondering whether it is worth introducing > > a new macro, for example, ARCH_HAVE_HPAGE_PMD_ORDER_MACRO (maybe not a > > good name), then we could make the code #ifdef the new macro. > > Likewise, I don't know if it's worth it or not. The config generating > the original -Wframe-larger-than= had 1024 nodes. If we use that as an > upper bound the number of nodes in a system then it's a difference of > 2 KiB / MADV_COLLAPSE call in the extreme case. Now, this is the first > time I've actually used kmalloc(), so I don't have a good sense of how > frugal we should be with memory. 2 KiB does sound like a lot - but I > suspect the common case will be < 32 bytes. Do we have a sense for the > largest sane CONFIG_NODES_SHIFT? The only systems I deal with are 1,2 > or 4 nodes, so I don't know what a sensible upper bound is here. CONFIG_NODES_SHIFT is 10 in my default config. So every kmalloc actually allocates 4K (u32) or 2K (u16). But 2K difference for each MADV_COLLAPSE call should be not a big deal on the machines that run THP. So it may be overkilling. Anyway I'm fine to the patch, you could have my Reviewed-by: Yang Shi <shy828301@gmail.com> > > > > > > > > > [1] https://lore.kernel.org/linux-mm/CAAa6QmQXn9odA-dsm-FFKqQmDvBuH0qDkmPFfXDS1so+Loe+4Q@mail.gmail.com/ > > > [2] https://lore.kernel.org/linux-mm/Ys2CeIm%2FQmQwWh9a@google.com/ > > > [3] https://lore.kernel.org/linux-next/Ytk0TQO9khUQgjxT@google.com/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition 2022-09-07 17:07 ` Yang Shi @ 2022-09-07 18:47 ` Zach O'Keefe 0 siblings, 0 replies; 7+ messages in thread From: Zach O'Keefe @ 2022-09-07 18:47 UTC (permalink / raw) To: Yang Shi; +Cc: Andrew Morton, linux-mm, kbuild-all On Wed, Sep 7, 2022 at 10:07 AM Yang Shi <shy828301@gmail.com> wrote: > > On Tue, Sep 6, 2022 at 1:28 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > On Tue, Sep 6, 2022 at 1:04 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > On Tue, Sep 6, 2022 at 12:14 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > > > > > On Tue, Sep 6, 2022 at 11:46 AM Yang Shi <shy828301@gmail.com> wrote: > > > > > > > > > > On Fri, Sep 2, 2022 at 7:12 PM Zach O'Keefe <zokeefe@google.com> wrote: > > > > > > > > > > > > HPAGE_PMD_ORDER is used to determine a minimal amount of bits needed to store > > > > > > per-node counters in struct collapse_control, defining the array to be either a > > > > > > u16[HPAGE_PMD_ORDER] or u32[HPAGE_PMD_ORDER]. > > > > > > > > > > > > For some architectures/configs, HPAGE_PMD_ORDER isn't resolved at preprocessor > > > > > > time (suitable for resolving preprocessor directives) but rather expands into a > > > > > > compiler built-in that is resolved at compile-time. E.g. for 32-bit MIPS (see > > > > > > link for full config): > > > > > > > > > > > > ---8<-- > > > > > > >> arch/mips/include/asm/pgtable.h:238:26: warning: "__builtin_ffs" is not defined, evaluates to 0 [-Wundef] > > > > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > > > > | ^~~~~~~~~~~~~ > > > > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > > > > | ^~~~~~~~~~ > > > > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > > > > | ^~~~~~~~~~~ > > > > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > > > > | ^~~~~~~~~ > > > > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > > > > | ^~~~~~~~~ > > > > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > > > > | ^~~~~~~~~ > > > > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > > > > | ^~~~~~~~~~~~~~~ > > > > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > > > > | ^~~~~~~~~~~~~~~ > > > > > > >> arch/mips/include/asm/pgtable.h:238:39: error: missing binary operator before token "(" > > > > > > 238 | #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) > > > > > > | ^ > > > > > > arch/mips/include/asm/pgtable-32.h:65:43: note: in expansion of macro 'PTE_T_LOG2' > > > > > > 65 | # define PGDIR_SHIFT (2 * PAGE_SHIFT - PTE_T_LOG2 - 1) > > > > > > | ^~~~~~~~~~ > > > > > > include/asm-generic/pgtable-nop4d.h:11:33: note: in expansion of macro 'PGDIR_SHIFT' > > > > > > 11 | #define P4D_SHIFT PGDIR_SHIFT > > > > > > | ^~~~~~~~~~~ > > > > > > include/asm-generic/pgtable-nopud.h:18:25: note: in expansion of macro 'P4D_SHIFT' > > > > > > 18 | #define PUD_SHIFT P4D_SHIFT > > > > > > | ^~~~~~~~~ > > > > > > include/asm-generic/pgtable-nopmd.h:20:25: note: in expansion of macro 'PUD_SHIFT' > > > > > > 20 | #define PMD_SHIFT PUD_SHIFT > > > > > > | ^~~~~~~~~ > > > > > > include/linux/huge_mm.h:109:25: note: in expansion of macro 'PMD_SHIFT' > > > > > > 109 | #define HPAGE_PMD_SHIFT PMD_SHIFT > > > > > > | ^~~~~~~~~ > > > > > > include/linux/huge_mm.h:105:26: note: in expansion of macro 'HPAGE_PMD_SHIFT' > > > > > > 105 | #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT) > > > > > > | ^~~~~~~~~~~~~~~ > > > > > > mm/khugepaged.c:90:5: note: in expansion of macro 'HPAGE_PMD_ORDER' > > > > > > 90 | #if HPAGE_PMD_ORDER < 16 > > > > > > | ^~~~~~~~~~~~~~~ > > > > > > ---8<-- > > > > > > > > > > > > load_node is already special-cased for CONFIG_PPC64 in > > > > > > "mm-khugepaged-add-struct-collapse_control-fix-fix", and we could continue down > > > > > > that path by #define'ing away edge cases, but I think at this point it easier > > > > > > to just let load_node be an array of u32s. If it's ever an issue, we can > > > > > > revisit. > > > > > > > > > > > > This essentially rolls back both: > > > > > > > > > > > > "mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR" > > > > > > "mm-khugepaged-add-struct-collapse_control-fix-fix" > > > > > > > > > > > > (though we'll leave it as a u32 vs an int). > > > > > > > > > > > > Link: https://lore.kernel.org/linux-mm/202209021349.F73i5d6X-lkp@intel.com/ > > > > > > Fixes: 5f7bb00bd9cd ("mm/khugepaged: use minimal bits to store num page < HPAGE_PMD_NR") > > > > > > Fixes: 03053c821af2 ("mm-khugepaged-add-struct-collapse_control-fix-fix") > > > > > > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > > > > > > --- > > > > > > mm/khugepaged.c | 7 +------ > > > > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > > > > > index af3b07eb2389..29142a35d24c 100644 > > > > > > --- a/mm/khugepaged.c > > > > > > +++ b/mm/khugepaged.c > > > > > > @@ -93,13 +93,8 @@ struct collapse_control { > > > > > > bool is_khugepaged; > > > > > > > > > > > > /* Num pages scanned per node */ > > > > > > -#if defined(CONFIG_PPC64) > > > > > > u32 node_load[MAX_NUMNODES]; > > > > > > -#elif HPAGE_PMD_ORDER < 16 > > > > > > - u16 node_load[MAX_NUMNODES]; > > > > > > -#else > > > > > > - u32 node_load[MAX_NUMNODES]; > > > > > > -#endif > > > > > > > > > > I recall we need this to circumvent some build time warning about > > > > > stack usage? Or I misremember? > > > > > > > > > > > > > Hey Yang, thanks for keeping a look out here, and good memory! > > > > > > > > The stack usage issue was due to struct collapse_control being > > > > allocated on the stack when MAX_NUMNODES was massive[1]. Now that we > > > > kmalloc() them for MADV_COLLAPSE, idea is we'd want to limit required > > > > memory and the original change here was an optimization due to an > > > > observation that we really didn't need 32 bits to hold a maximum value > > > > of HPAGE_PMD_NR[2], but determining some safe upper bound for the > > > > number of requires bits per arch/config as been a bit of a pain, with > > > > special casing adding to support POWER64 already[3], and now this mips > > > > issue that would otherwise require something special. > > > > > > Aha, thanks for refreshing my memory. > > > > > > The patch is fine to me. I'm wondering whether it is worth introducing > > > a new macro, for example, ARCH_HAVE_HPAGE_PMD_ORDER_MACRO (maybe not a > > > good name), then we could make the code #ifdef the new macro. > > > > Likewise, I don't know if it's worth it or not. The config generating > > the original -Wframe-larger-than= had 1024 nodes. If we use that as an > > upper bound the number of nodes in a system then it's a difference of > > 2 KiB / MADV_COLLAPSE call in the extreme case. Now, this is the first > > time I've actually used kmalloc(), so I don't have a good sense of how > > frugal we should be with memory. 2 KiB does sound like a lot - but I > > suspect the common case will be < 32 bytes. Do we have a sense for the > > largest sane CONFIG_NODES_SHIFT? The only systems I deal with are 1,2 > > or 4 nodes, so I don't know what a sensible upper bound is here. > > CONFIG_NODES_SHIFT is 10 in my default config. [..] Good to know! Thank you! > [..] So every kmalloc > actually allocates 4K (u32) or 2K (u16). But 2K difference for each > MADV_COLLAPSE call should be not a big deal on the machines that run > THP. So it may be overkilling. SG - can always change it later if it proves to be a lot / if we have many MADV_COLLAPSE calls in flight. > Anyway I'm fine to the patch, you could have my Reviewed-by: Yang Shi > <shy828301@gmail.com> Thanks as always! Zach > > > > > > > > > > > > > [1] https://lore.kernel.org/linux-mm/CAAa6QmQXn9odA-dsm-FFKqQmDvBuH0qDkmPFfXDS1so+Loe+4Q@mail.gmail.com/ > > > > [2] https://lore.kernel.org/linux-mm/Ys2CeIm%2FQmQwWh9a@google.com/ > > > > [3] https://lore.kernel.org/linux-next/Ytk0TQO9khUQgjxT@google.com/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-07 18:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-03 2:12 [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition Zach O'Keefe 2022-09-06 18:46 ` Yang Shi 2022-09-06 19:13 ` Zach O'Keefe 2022-09-06 20:04 ` Yang Shi 2022-09-06 20:27 ` Zach O'Keefe 2022-09-07 17:07 ` Yang Shi 2022-09-07 18:47 ` Zach O'Keefe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox