* [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 related [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;
as well as URLs for NNTP newsgroup(s).