From: Yang Shi <shy828301@gmail.com>
To: "Zach O'Keefe" <zokeefe@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
linux-mm@kvack.org, kbuild-all@lists.01.org
Subject: Re: [PATCH mm-unstable] mm/khugepaged: fix struct collapse_control load_node definition
Date: Tue, 6 Sep 2022 11:46:28 -0700 [thread overview]
Message-ID: <CAHbLzkpVq5Fu0doYVLwRL4sRzTrEPoeg0WQ1wMdtBE==dAzvxA@mail.gmail.com> (raw)
In-Reply-To: <20220903021221.1130021-1-zokeefe@google.com>
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
>
next prev parent reply other threads:[~2022-09-06 18:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHbLzkpVq5Fu0doYVLwRL4sRzTrEPoeg0WQ1wMdtBE==dAzvxA@mail.gmail.com' \
--to=shy828301@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=kbuild-all@lists.01.org \
--cc=linux-mm@kvack.org \
--cc=zokeefe@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).