Re: [PATCH v3] mm/page_alloc: Require pahole v1.22 to cope with zero-sized struct pagesets

[Mel Gorman]

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@xxxxxxx>
> > Reported-by: Hritik Vijay <hritikxx8@xxxxxxxxx>
> > Debugged-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> > Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> > ---
> 
> 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.

-- 
Mel Gorman
SUSE Labs