On Tue, Sep 6, 2022 at 12:14 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> wrote:
>
> On Tue, Sep 6, 2022 at 11:46 AM Yang Shi <shy828301@xxxxxxxxx> wrote:
> >
> > On Fri, Sep 2, 2022 at 7:12 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> 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@xxxxxxxxx/
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/linux-mm/Ys2CeIm%2FQmQwWh9a@xxxxxxxxxx/
> [3] https://lore.kernel.org/linux-next/Ytk0TQO9khUQgjxT@xxxxxxxxxx/
