An additional report: https://lore.kernel.org/lkml/20210623223015.GA315292@paulmck-ThinkPad-P17-Ge... EOM
On Fri, Jun 18, 2021 at 4:05 PM Nathan Chancellor nathan@kernel.org wrote:
On Fri, Jun 18, 2021 at 10:32:42AM +0530, Aneesh Kumar K.V wrote:
On 6/17/21 11:32 PM, Nathan Chancellor wrote:
Rebuilt the CC list because most people were added based on the incorrect bisect result.
On Thu, Jun 17, 2021 at 02:51:49PM +0100, Matthew Wilcox wrote:
On Thu, Jun 17, 2021 at 06:15:45PM +0530, Naresh Kamboju wrote:
On Thu, 17 Jun 2021 at 17:41, Naresh Kamboju naresh.kamboju@linaro.org wrote:
x86_64-linux-gnu-ld: mm/mremap.o: in function `move_pgt_entry': mremap.c:(.text+0x763): undefined reference to `__compiletime_assert_342'
The git bisect pointed out the first bad commit.
The first bad commit: commit 928cf6adc7d60c96eca760c05c1000cda061604e Author: Stephen Boyd swboyd@chromium.org Date: Thu Jun 17 15:21:35 2021 +1000 module: add printk formats to add module build ID to stacktraces
Your git bisect probably went astray. There's no way that commit caused that regression.
My bisect landed on commit 83f85ac75855 ("mm/mremap: convert huge PUD move to separate helper"). flush_pud_tlb_range() evaluates to BUILD_BUG() when CONFIG_TRANSPARENT_HUGEPAGE is unset but this function is present just based on the value of CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD.
$ make -skj(nproc) ARCH=x86_64 CC=clang O=build/x86_64 distclean allnoconfig mm/mremap.o
$ llvm-readelf -s build/x86_64/mm/mremap.o &| rg __compiletime_assert 21: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __compiletime_assert_337
$ rg TRANSPARENT_ build/x86_64/.config 450:CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE=y 451:CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD=y 562:# CONFIG_TRANSPARENT_HUGEPAGE is not set
Not sure why this does not happen on newer clang versions, presumably something with inlining decisions? Still seems like a legitimate issue to me.
gcc 10 also doesn't give a build error. I guess that is because we evaluate
if (pud_trans_huge(*old_pud) || pud_devmap(*old_pud)) {to if (0) with CONFIG_TRANSPARENT_HUGEPAGE disabled.
switching that to if (1) do results in BUILD_BUG triggering.
Thanks for pointing that out. I think what happens with clang-10 and clang-11 is that move_huge_pud() gets inlined into move_pgt_entry() but then the compiler does not figure out that the HPAGE_PUD case is dead so the code sticks around, where as GCC and newer clang versions can figure that out and eliminate that case.
Should we fix this ?
Yes, I believe that we should.
modified mm/mremap.c @@ -336,7 +336,7 @@ static inline bool move_normal_pud(struct vm_area_struct *vma, } #endif
-#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD +#if defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD) && defined(CONFIG_TRANSPARENT_HUGEPAGE) static bool move_huge_pud(struct vm_area_struct *vma, unsigned long old_addr, unsigned long new_addr, pud_t *old_pud, pud_t *new_pud) {
That works or we could mirror what has already been done for the HPAGE_PMD case. No personal preference.
diff --git a/mm/mremap.c b/mm/mremap.c index 9a7fbec31dc9..5989d3990020 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -460,7 +460,8 @@ static bool move_pgt_entry(enum pgt_entry entry, struct vm_area_struct *vma, new_entry); break; case HPAGE_PUD:
moved = move_huge_pud(vma, old_addr, new_addr, old_entry,
moved = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&move_huge_pud(vma, old_addr, new_addr, old_entry, new_entry); break;Cheers, Nathan