From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 24EE02F22; Mon, 12 Dec 2022 17:05:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 28B21C433F0; Mon, 12 Dec 2022 17:05:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1670864704; bh=KmG4ro7h+VdOXjEslNq+8iBWKBTQ/RA7zskQtxL+RXw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Aj6vrtB3Y2skOx8SKPJd7krfhSKJ/utA+bWT4xTHarHMUD3/lISCtlvJqPHL60pzb gBefVSEqHgMTU2td3vxDOudSSqtIFyGEE3fjACq1ImpoFS6AbTSU4gyoNzKLtYmaUk 8HFKc5bYSKCl0NPKxs5dwH01VjTI/AU8U1qEI6cnPh7ywjBflGg9Adm5tMcwnxVY3Q IzeAVMFpIdKeFQBwv3sUlGm3hpuFpURSm4Di7el+3ELuoTPiN3B+mdwleD0ujL+D77 U47NENYSq9mwaTnfNquImIqO6NV+79DE502FuufTe2OhE4tXY8SzjLVoqYzGx34tUW xOp0Is+zD4lrA== Date: Mon, 12 Dec 2022 10:05:02 -0700 From: Nathan Chancellor To: Masahiro Yamada Cc: Nick Desaulniers , Tom Rix , Nicolas Schier , Sami Tolvanen , Vincent Donnefort , linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org, llvm@lists.linux.dev, patches@lists.linux.dev, Daniel Jordan , Steffen Klassert , linux-crypto@vger.kernel.org Subject: Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref Message-ID: References: <20221207191657.2852229-1-nathan@kernel.org> <20221207191657.2852229-2-nathan@kernel.org> Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Mon, Dec 12, 2022 at 10:07:24PM +0900, Masahiro Yamada wrote: > On Thu, Dec 8, 2022 at 4:17 AM Nathan Chancellor wrote: > > > > When building arm64 allmodconfig + ThinLTO with clang and a proposed > > modpost update to account for -ffuncton-sections, the following warning > > appears: > > > > WARNING: modpost: vmlinux.o: section mismatch in reference: padata_work_init (section: .text.padata_work_init) -> padata_mt_helper (section: .init.text) > > WARNING: modpost: vmlinux.o: section mismatch in reference: padata_work_init (section: .text.padata_work_init) -> padata_mt_helper (section: .init.text) > > > > LLVM has optimized padata_work_init() to include the address of > > padata_mt_helper() directly, which causes modpost to complain since > > padata_work_init() is not __init, whereas padata_mt_helper() is. In > > reality, padata_work_init() is only called with padata_mt_helper() as > > the work_fn argument in code that is __init, so this warning will not > > result in any problems. Silence it with __ref, which makes it clear to > > modpost that padata_work_init() can only use padata_mt_helper() in > > __init code. > > > > Suggested-by: Daniel Jordan > > Signed-off-by: Nathan Chancellor > > --- > > Cc: Steffen Klassert > > Cc: Daniel Jordan > > Cc: linux-crypto@vger.kernel.org > > --- > > kernel/padata.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/padata.c b/kernel/padata.c > > index e5819bb8bd1d..4c3137fe8449 100644 > > --- a/kernel/padata.c > > +++ b/kernel/padata.c > > @@ -83,8 +83,8 @@ static struct padata_work *padata_work_alloc(void) > > return pw; > > } > > > > -static void padata_work_init(struct padata_work *pw, work_func_t work_fn, > > - void *data, int flags) > > +static __ref void padata_work_init(struct padata_work *pw, work_func_t work_fn, > > + void *data, int flags) > > { > > if (flags & PADATA_WORK_ONSTACK) > > INIT_WORK_ONSTACK(&pw->pw_work, work_fn); > > > > base-commit: 76dcd734eca23168cb008912c0f69ff408905235 > > -- > > 2.38.1 > > > > It took me a while to understand why LTO can embed > padata_mt_helper's address into padata_work_init(). Sorry about that, I can try to expand on this in both the commit message and in-code comment if I end up adding it. > There are 3 call-sites to padata_work_init(). > > (1) __init padata_work_alloc_mt() > --> padata_work_init(..., padata_mt_helper, ...) > > (2) padata_do_parallel() > --> padata_work_init(..., padata_parallel_worker, ...) > > (3) __init padata_do_multithreaded() > --> padata_work_init(..., padata_mt_helper, ...) > > > The function call (2) is squashed away. > > > With only (1) and (3) remaining, the 2nd parameter to > padata_work_init() is always padata_mt_helper, > therefore LLVM embeds padata_mt_hlper's address > directly into padata_work_init(). > > I am not sure if the compiler should do this level of optimization > because kernel/padata.c does not seem to be a special case. > Perhaps, we might be hit with more cases that need __ref annotation, > which is only required by LTO. That's possible. I did only see this once instance in all my builds but allmodconfig + ThinLTO might not be too interesting of a case, since the sanitizers will be enabled, which makes optimization more difficult. I could try to enable ThinLTO with some distribution configurations to see if there are any more instances that crop up. > One note is that, we could discard padata_work_init() > because (1) and (3) are both annotated as __init. > So, another way of fixing is > static __always_inline void padata_work_init(...) > because the compiler would determine padata_work_init() > would be small enough if the caller and callee belonged to > the same section. > > I do not have a strong opinion. > Honestly, I do not know what the best approach would be to fix this. Agreed to both points, it is really up to the padata maintainers. > If we go with the __ref annotation, I can pick this, but > at least can you add some comments? > > > include/linux/init.h says: > "optimally document why the __ref is needed and why it's OK" > > > I think this is the case that needs some comments > because LTO optimization looks too tricky to me. Sure thing, I will send a v3 either Tuesday or Wednesday with an updated commit message and code comment if we end up going this route. Thank you for the review! Cheers, Nathan