Date:   Thu, 15 Jul 2021 21:44:53 +0200
From:   Michal Suchánek <msuchanek@...e.de>
To:     Mel Gorman <mgorman@...hsingularity.net>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Hellwig <hch@...radead.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Jiri Olsa <jolsa@...nel.org>,
        Hritik Vijay <hritikxx8@...il.com>,
        Linux-BPF <bpf@...r.kernel.org>,
        Linux-Net <netdev@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>, clm@...com
Subject: Re: [PATCH v3] mm/page_alloc: Require pahole v1.22 to cope with
 zero-sized struct pagesets

Hello,

On Mon, May 31, 2021 at 10:35:54AM +0100, Mel Gorman wrote:
> On Sat, May 29, 2021 at 08:10:36PM -0700, Andrii Nakryiko wrote:
> > On Fri, May 28, 2021 at 12:42 AM Mel Gorman <mgorman@...hsingularity.net> wrote:
> > >
> > > On Thu, May 27, 2021 at 03:17:48PM -0700, Andrii Nakryiko wrote:
> > > > > Andrii Nakryiko bisected the problem to the commit "mm/page_alloc: convert
> > > > > per-cpu list protection to local_lock" currently staged in mmotm. In his
> > > > > own words
> > > > >
> > > > >   The immediate problem is two different definitions of numa_node per-cpu
> > > > >   variable. They both are at the same offset within .data..percpu ELF
> > > > >   section, they both have the same name, but one of them is marked as
> > > > >   static and another as global. And one is int variable, while another
> > > > >   is struct pagesets. I'll look some more tomorrow, but adding Jiri and
> > > > >   Arnaldo for visibility.
> > > > >
> > > > >   [110907] DATASEC '.data..percpu' size=178904 vlen=303
> > > > >   ...
> > > > >         type_id=27753 offset=163976 size=4 (VAR 'numa_node')
> > > > >         type_id=27754 offset=163976 size=4 (VAR 'numa_node')
> > > > >
> > > > >   [27753] VAR 'numa_node' type_id=27556, linkage=static
> > > > >   [27754] VAR 'numa_node' type_id=20, linkage=global
> > > > >
> > > > >   [20] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >
> > > > >   [27556] STRUCT 'pagesets' size=0 vlen=1
> > > > >         'lock' type_id=507 bits_offset=0
> > > > >
> > > > >   [506] STRUCT '(anon)' size=0 vlen=0
> > > > >   [507] TYPEDEF 'local_lock_t' type_id=506
> > > > >
> > > > > The patch in question introduces a zero-sized per-cpu struct and while
> > > > > this is not wrong, versions of pahole prior to 1.22 get confused during
> > > > > BTF generation with two separate variables occupying the same address.
> > > > >
> > > > > This patch adds a requirement for pahole 1.22 before setting
> > > > > DEBUG_INFO_BTF.  While pahole 1.22 does not exist yet, a fix is in the
> > > > > pahole git tree as ("btf_encoder: fix and complete filtering out zero-sized
> > > > > per-CPU variables").
> > > > >
> > > > > Reported-by: Michal Suchanek <msuchanek@...e.de>
> > > > > Reported-by: Hritik Vijay <hritikxx8@...il.com>
> > > > > Debugged-by: Andrii Nakryiko <andrii.nakryiko@...il.com>
> > > > > Signed-off-by: Mel Gorman <mgorman@...hsingularity.net>
> > > > > ---
> > > >
> > > > I still think that v1 ([0]) is a more appropriate temporary solution
> > > > until pahole 1.22 is released and widely packaged. Suddenly raising
> > > > the minimum version to 1.22, which is not even released even, is a
> > > > pretty big compatibility concern for all the users that rely on
> > > > CONFIG_DEBUG_INFO_BTF.
> > >
> > > On the flip side, we have a situation where a build tool (pahole) has a
> > > problem whereby correct code does not result in a working kernel. It's
> > > not that dissimilar to preventing the kernel being built on an old
> > > compiler. While I accept it's unfortunate, Christoph had a point where
> > > introducing workarounds in the kernel could lead to a prolification of
> > > workarounds for pahole or other reasons that are potentially tricky to
> > > revert as long as distributions exist that do not ship with a sufficiently
> > > reason package.
> > >
> > > > Just a few days ago pahole 1.16 worked fine and
> > > > here we suddenly (and silently due to how Kconfig functions) raise
> > > > that to a version that doesn't exist. That's going to break workflows
> > > > for a lot of people.
> > > >
> > >
> > > People do have a workaround though. For the system building the kernel,
> > > they can patch pahole and revert the check so a bootable kernel can be
> > > built. It's not convenient but it is manageable and pahole has until
> > > 5.13 releases to release a v1.22. The downsides for the alternative --
> > > a non-booting kernel are much more severe.
> > >
> > > > I'm asking to have that ugly work-around to ensure sizeof(struct
> > > > pagesets) > 0 as a temporary solution only.
> > >
> > > Another temporary solution is to locally build pahole and either revert
> > > the check or fake the 1.22 release number with the self-built pahole.
> > >
> > 
> > Well, luckily it seems we anticipated issues like that and added
> > --skip_encoding_btf_vars argument, which I completely forgot about and
> > just accidentally came across reviewing Arnaldo's latest pahole patch.
> > I think that one is a much better solution, as then it will impact
> > only those that explicitly relies on availability of BTF for per-CPU
> > variables, which is a subset of all possible uses for kernel BTF. Sent
> > a patch ([0]), please take a look.
> > 
> >   [0] https://lore.kernel.org/linux-mm/20210530002536.3193829-1-andrii@kernel.org/T/#u
> 
> I'm happy to have this patch used as an alternative to forcing 1.22 to
> be the minimum version of pahole required.

Is pahole 1.22 available already?

Adding the a patch that reports different version is kind of annoying.

Thanks

Michal

Powered by blists - more mailing lists