Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <202301170941.49728982-oliver.sang@intel.com> <2f483247-da76-9ec9-3e51-f690939f4585@suse.cz>
 <Y8ZVxJSZdtEk8Nco@feng-clx> <Y8aSc5xGO+rW2pyo@feng-clx>
In-Reply-To: <Y8aSc5xGO+rW2pyo@feng-clx>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 17 Jan 2023 10:25:17 -0800
Message-ID: <CAHk-=wiS84nS9apjs_Vt=jjZ_+j+F8HQ3B+ABSvbzcqtW9x5Kg@mail.gmail.com>
Subject: Re: [linus:master] [hugetlb] 7118fc2906: kernel_BUG_at_lib/list_debug.c
To:     Feng Tang <feng.tang@intel.com>
Cc:     Vlastimil Babka <vbabka@suse.cz>,
        "Sang, Oliver" <oliver.sang@intel.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        "oe-lkp@lists.linux.dev" <oe-lkp@lists.linux.dev>,
        lkp <lkp@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jann Horn <jannh@google.com>,
        "Song, Youquan" <youquan.song@intel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Jan Kara <jack@suse.cz>, John Hubbard <jhubbard@nvidia.com>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Matthew Wilcox <willy@infradead.org>,
        Michal Hocko <mhocko@kernel.org>,
        Muchun Song <songmuchun@bytedance.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        "Yin, Fengwei" <fengwei.yin@intel.com>, hongjiu.lu@intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 17, 2023 at 4:22 AM Feng Tang <feng.tang@intel.com> wrote:
>
> With the following patch to use 'O1' instead 'O2' gcc optoin for
> page_alloc.c, the list corruption issue can't be reproduced for
> commit 7118fc2906 in 1000 runs.

Ugh.

It would be lovely if you could just narrow it down with

  #pragma GCC optimize ("O1")
 ...
  #pragma GCC optimize ("O2")

around just that prep_compound_page(), but when I tried it myself I
get some function attribute mismatch errors.


> As is can't be reproduced with X86_64 build, it could be i386
> compiling related.

Your particular config causes a huge amount of nasty 64-bit arithmetic
according to the objdump code, with sequences like

  c13b3cbb:       83 05 d0 28 6c c5 01    addl   $0x1,0xc56c28d0
  c13b3cc2:       83 15 d4 28 6c c5 00    adcl   $0x0,0xc56c28d4

which seems to be just from some coverage profiling being on
(CONFIG_GCOV?), or something. It makes it very hard to read the code.

You also have UBSAN enabled, which - again - makes for some really
grotty asm that hides any actual logic.

Finally, your objdump version also does some horrendous decoding, like

  c13b3e29:       8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi

which is just a 7-byte 'nop' instruction, but again, it makes it
rather hard to actually read the code.

With the i386 defconfig, gcc generates a function that is just ~30
instructions for me, so this makes a huge difference in the legibility
of the code.

I wonder if you can recreate the issue with a much more
straightforward config. By all means, leave DEBUG_PAGEALLOC and SLUB
debugging on, but without the things like UBSAN and GCOV.

> I also objdumped 'prep_compound_page' for vmlinux of 7118fc2906 and
> its parent commit 48b8d744ea84, which have big difference than the
> simple 'set_page_count()' change, but I can't tell which part is
> abnormal, so attach them for further check.

Yeah, I can't make heads or tails of them either, see above on how
illegible the objdump files are. And that's despite not even having
all of prep_compound_page() in them (it's missing
prep_compound_page.cold, which is probably just UBSAN fixup code, but
who knows..)

That said, with the i386 defconfig, the only change from adding
set_page_count() to the loop seems to be exactly that:

 .L589:
-       movl    $1024, 12(%eax)
+       movl    $0, 28(%eax)
        addl    $32, %eax
+       movl    $1024, -20(%eax)
        movl    %esi, -28(%eax)
        movl    $0, -12(%eax)
        cmpl    %edx, %eax

(don't ask me why gcc does *one* access using the pre-incremented
pointer, and then the rest to the post-incremented ones, but whatever
- it means that it's not just "add a mov $0", it's also changing how
the

        p->mapping = TAIL_MAPPING;

instruction is done, which is that

-       movl    $1024, 12(%eax)
+       movl    $1024, -20(%eax)

part of the change)

             Linus