From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86037C47082 for ; Mon, 31 May 2021 09:36:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 61BE36102A for ; Mon, 31 May 2021 09:36:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231231AbhEaJhn (ORCPT ); Mon, 31 May 2021 05:37:43 -0400 Received: from outbound-smtp10.blacknight.com ([46.22.139.15]:41553 "EHLO outbound-smtp10.blacknight.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231172AbhEaJhg (ORCPT ); Mon, 31 May 2021 05:37:36 -0400 Received: from mail.blacknight.com (pemlinmail03.blacknight.ie [81.17.254.16]) by outbound-smtp10.blacknight.com (Postfix) with ESMTPS id 5D72B1C4010 for ; Mon, 31 May 2021 10:35:56 +0100 (IST) Received: (qmail 12829 invoked from network); 31 May 2021 09:35:56 -0000 Received: from unknown (HELO techsingularity.net) (mgorman@techsingularity.net@[84.203.17.255]) by 81.17.254.9 with ESMTPSA (AES256-SHA encrypted, authenticated); 31 May 2021 09:35:56 -0000 Date: Mon, 31 May 2021 10:35:54 +0100 From: Mel Gorman To: Andrii Nakryiko Cc: Andrew Morton , Christoph Hellwig , Arnaldo Carvalho de Melo , Michal Suchanek , Alexei Starovoitov , Daniel Borkmann , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , LKML , Jiri Olsa , Hritik Vijay , Linux-BPF , Linux-Net , Linux-MM , clm@fb.com Subject: Re: [PATCH v3] mm/page_alloc: Require pahole v1.22 to cope with zero-sized struct pagesets Message-ID: <20210531093554.GT30378@techsingularity.net> References: <20210527171923.GG30378@techsingularity.net> <20210528074248.GI30378@techsingularity.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, May 29, 2021 at 08:10:36PM -0700, Andrii Nakryiko wrote: > On Fri, May 28, 2021 at 12:42 AM Mel Gorman 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 > > > > Reported-by: Hritik Vijay > > > > Debugged-by: Andrii Nakryiko > > > > Signed-off-by: Mel Gorman > > > > --- > > > > > > 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. -- Mel Gorman SUSE Labs