* [PATCH v2 0/2] Fix lack of section mismatch warnings with LTO @ 2022-12-07 19:16 Nathan Chancellor 2022-12-07 19:16 ` [PATCH v2 1/2] padata: Mark padata_work_init() as __ref Nathan Chancellor 2022-12-07 19:16 ` [PATCH v2 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor 0 siblings, 2 replies; 10+ messages in thread From: Nathan Chancellor @ 2022-12-07 19:16 UTC (permalink / raw) To: Masahiro Yamada Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Nathan Chancellor, Steffen Klassert, Daniel Jordan, linux-crypto Hi all, Vincent recently reported an issue with lack of section mismatch warnings with LTO. This is due to commit 6c730bfc894f ("modpost: handle -ffunction-sections"), which ignores all function sections for modpost. I believe this is incorrect, as these function sections may still refer to symbols in other sections and they will ultimately be coalesced into .text by vmlinux.lds anyways. The first patch fixes a warning that I see with allmodconfig + ThinLTO builds after applying the second patch. The second patch moves ".text.*" into TEXT_SECTIONS so that modpost audits them for mismatches. I expect this to go via the kbuild tree with an ack from the padata maintainers. Cc: Steffen Klassert <steffen.klassert@secunet.com> Cc: Daniel Jordan <daniel.m.jordan@oracle.com> Cc: linux-crypto@vger.kernel.org v2: - Rework first patch to use __ref for padata_work_init() so that padata_mt_helper() can remain __init (Daniel). Sami, I did not include your tag on this one because it was different enough from what you initially reviewed. - Pick up Alexander's, Sami's, and Vincent's tags on patch 2. v1: https://lore.kernel.org/20221129190123.872394-1-nathan@kernel.org/ Nathan Chancellor (2): padata: Mark padata_work_init() as __ref modpost: Include '.text.*' in TEXT_SECTIONS kernel/padata.c | 4 ++-- scripts/mod/modpost.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) base-commit: 76dcd734eca23168cb008912c0f69ff408905235 -- 2.38.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] padata: Mark padata_work_init() as __ref 2022-12-07 19:16 [PATCH v2 0/2] Fix lack of section mismatch warnings with LTO Nathan Chancellor @ 2022-12-07 19:16 ` Nathan Chancellor 2022-12-07 19:56 ` Daniel Jordan 2022-12-12 13:07 ` Masahiro Yamada 2022-12-07 19:16 ` [PATCH v2 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor 1 sibling, 2 replies; 10+ messages in thread From: Nathan Chancellor @ 2022-12-07 19:16 UTC (permalink / raw) To: Masahiro Yamada Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Nathan Chancellor, Daniel Jordan, Steffen Klassert, linux-crypto 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 <daniel.m.jordan@oracle.com> Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- Cc: Steffen Klassert <steffen.klassert@secunet.com> Cc: Daniel Jordan <daniel.m.jordan@oracle.com> 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref 2022-12-07 19:16 ` [PATCH v2 1/2] padata: Mark padata_work_init() as __ref Nathan Chancellor @ 2022-12-07 19:56 ` Daniel Jordan 2022-12-12 13:07 ` Masahiro Yamada 1 sibling, 0 replies; 10+ messages in thread From: Daniel Jordan @ 2022-12-07 19:56 UTC (permalink / raw) To: Nathan Chancellor Cc: Masahiro Yamada, Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Steffen Klassert, linux-crypto On Wed, Dec 07, 2022 at 12:16:56PM -0700, 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. Thanks! Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com> > Suggested-by: Daniel Jordan <daniel.m.jordan@oracle.com> > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: Daniel Jordan <daniel.m.jordan@oracle.com> > 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref 2022-12-07 19:16 ` [PATCH v2 1/2] padata: Mark padata_work_init() as __ref Nathan Chancellor 2022-12-07 19:56 ` Daniel Jordan @ 2022-12-12 13:07 ` Masahiro Yamada 2022-12-12 17:05 ` Nathan Chancellor 1 sibling, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2022-12-12 13:07 UTC (permalink / raw) To: Nathan Chancellor Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Daniel Jordan, Steffen Klassert, linux-crypto On Thu, Dec 8, 2022 at 4:17 AM Nathan Chancellor <nathan@kernel.org> 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 <daniel.m.jordan@oracle.com> > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: Daniel Jordan <daniel.m.jordan@oracle.com> > 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(). 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. 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. 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. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref 2022-12-12 13:07 ` Masahiro Yamada @ 2022-12-12 17:05 ` Nathan Chancellor 2022-12-12 19:21 ` Daniel Jordan 0 siblings, 1 reply; 10+ messages in thread From: Nathan Chancellor @ 2022-12-12 17:05 UTC (permalink / raw) To: Masahiro Yamada Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Daniel Jordan, Steffen Klassert, linux-crypto On Mon, Dec 12, 2022 at 10:07:24PM +0900, Masahiro Yamada wrote: > On Thu, Dec 8, 2022 at 4:17 AM Nathan Chancellor <nathan@kernel.org> 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 <daniel.m.jordan@oracle.com> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > --- > > Cc: Steffen Klassert <steffen.klassert@secunet.com> > > Cc: Daniel Jordan <daniel.m.jordan@oracle.com> > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref 2022-12-12 17:05 ` Nathan Chancellor @ 2022-12-12 19:21 ` Daniel Jordan 2022-12-12 20:06 ` Nathan Chancellor 0 siblings, 1 reply; 10+ messages in thread From: Daniel Jordan @ 2022-12-12 19:21 UTC (permalink / raw) To: Nathan Chancellor Cc: Masahiro Yamada, Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Steffen Klassert, linux-crypto On Mon, Dec 12, 2022 at 10:05:02AM -0700, Nathan Chancellor wrote: > On Mon, Dec 12, 2022 at 10:07:24PM +0900, Masahiro Yamada wrote: > > 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. Yes, if there were many more instances of this problem it might be worth thinking about an LTO-specific solution to fix it closer to the source. > > 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. I'm right there with you. :-) > > Honestly, I do not know what the best approach would be to fix this. Either approach works, either can include an explanatory comment. __ref seems more targeted to the problem at hand. > > 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. A nitpick, but as long as you're respinning, if we stay with this approach, could you put __ref just before the function name? init.h says "The markers follow same syntax rules as __init / __initdata" and for those it says "You should add __init immediately before the function name" though there are plenty of places in the tree that don't do this. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref 2022-12-12 19:21 ` Daniel Jordan @ 2022-12-12 20:06 ` Nathan Chancellor 2022-12-13 16:48 ` Nathan Chancellor 0 siblings, 1 reply; 10+ messages in thread From: Nathan Chancellor @ 2022-12-12 20:06 UTC (permalink / raw) To: Daniel Jordan Cc: Masahiro Yamada, Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Steffen Klassert, linux-crypto On Mon, Dec 12, 2022 at 02:21:57PM -0500, Daniel Jordan wrote: > On Mon, Dec 12, 2022 at 10:05:02AM -0700, Nathan Chancellor wrote: > > On Mon, Dec 12, 2022 at 10:07:24PM +0900, Masahiro Yamada wrote: > > > 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. > > Yes, if there were many more instances of this problem it might be worth > thinking about an LTO-specific solution to fix it closer to the source. Ack, I will wire up some build tests to see if this optimization occurs frequently enough to warrant a wider fix. > > > 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. > > I'm right there with you. :-) > > > > Honestly, I do not know what the best approach would be to fix this. > > Either approach works, either can include an explanatory comment. > __ref seems more targeted to the problem at hand. Right, I suspect __ref is the right way to go for this particular issue. I will add a comment regardless. > > > 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. > > A nitpick, but as long as you're respinning, if we stay with this > approach, could you put __ref just before the function name? init.h > says "The markers follow same syntax rules as __init / __initdata" and > for those it says "You should add __init immediately before the function > name" though there are plenty of places in the tree that don't do this. Sure thing! Cheers, Nathan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref 2022-12-12 20:06 ` Nathan Chancellor @ 2022-12-13 16:48 ` Nathan Chancellor 0 siblings, 0 replies; 10+ messages in thread From: Nathan Chancellor @ 2022-12-13 16:48 UTC (permalink / raw) To: Daniel Jordan Cc: Masahiro Yamada, Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Steffen Klassert, linux-crypto On Mon, Dec 12, 2022 at 01:06:16PM -0700, Nathan Chancellor wrote: > On Mon, Dec 12, 2022 at 02:21:57PM -0500, Daniel Jordan wrote: > > On Mon, Dec 12, 2022 at 10:05:02AM -0700, Nathan Chancellor wrote: > > > On Mon, Dec 12, 2022 at 10:07:24PM +0900, Masahiro Yamada wrote: > > > > 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. > > > > Yes, if there were many more instances of this problem it might be worth > > thinking about an LTO-specific solution to fix it closer to the source. > > Ack, I will wire up some build tests to see if this optimization occurs > frequently enough to warrant a wider fix. Turns out this does not appear to happen often. I built several distribution configurations for arm64 and x86_64 with CONFIG_LTO_CLANG_THIN=y and saw no modpost warnings. So I think this is sufficiently odd to keep the fix local to this one instance. I will send a v3 later today. > > > > 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. > > > > I'm right there with you. :-) > > > > > > Honestly, I do not know what the best approach would be to fix this. > > > > Either approach works, either can include an explanatory comment. > > __ref seems more targeted to the problem at hand. > > Right, I suspect __ref is the right way to go for this particular issue. > I will add a comment regardless. > > > > > 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. > > > > A nitpick, but as long as you're respinning, if we stay with this > > approach, could you put __ref just before the function name? init.h > > says "The markers follow same syntax rules as __init / __initdata" and > > for those it says "You should add __init immediately before the function > > name" though there are plenty of places in the tree that don't do this. > > Sure thing! > > Cheers, > Nathan > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] modpost: Include '.text.*' in TEXT_SECTIONS 2022-12-07 19:16 [PATCH v2 0/2] Fix lack of section mismatch warnings with LTO Nathan Chancellor 2022-12-07 19:16 ` [PATCH v2 1/2] padata: Mark padata_work_init() as __ref Nathan Chancellor @ 2022-12-07 19:16 ` Nathan Chancellor 2022-12-12 13:13 ` Masahiro Yamada 1 sibling, 1 reply; 10+ messages in thread From: Nathan Chancellor @ 2022-12-07 19:16 UTC (permalink / raw) To: Masahiro Yamada Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Nathan Chancellor, Alexander Lobakin Commit 6c730bfc894f ("modpost: handle -ffunction-sections") added ".text.*" to the OTHER_TEXT_SECTIONS macro to fix certain section mismatch warnings. Unfortunately, this makes it impossible for modpost to warn about section mismatchs with LTO, which implies '-ffunction-sections', as all functions are put in their own '.text.<func_name>' sections, which may still reference functions in sections they are not supposed to, such as __init. Fix this by moving ".text.*" into TEXT_SECTIONS, so that configurations with '-ffunction-sections' will see warnings about mismatched sections. Link: https://lore.kernel.org/Y39kI3MOtVI5BAnV@google.com/ Reported-by: Vincent Donnefort <vdonnefort@google.com> Reviewed-and-tested-by: Alexander Lobakin <alexandr.lobakin@intel.com> Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Tested-by: Vincent Donnefort <vdonnefort@google.com> Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- scripts/mod/modpost.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 2c80da0220c3..c861beabc128 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -823,10 +823,10 @@ static void check_section(const char *modname, struct elf_info *elf, #define ALL_EXIT_SECTIONS EXIT_SECTIONS, ALL_XXXEXIT_SECTIONS #define DATA_SECTIONS ".data", ".data.rel" -#define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \ +#define TEXT_SECTIONS ".text", ".text.*", ".sched.text", \ ".kprobes.text", ".cpuidle.text", ".noinstr.text" #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \ - ".fixup", ".entry.text", ".exception.text", ".text.*", \ + ".fixup", ".entry.text", ".exception.text", \ ".coldtext", ".softirqentry.text" #define INIT_SECTIONS ".init.*" -- 2.38.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] modpost: Include '.text.*' in TEXT_SECTIONS 2022-12-07 19:16 ` [PATCH v2 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor @ 2022-12-12 13:13 ` Masahiro Yamada 0 siblings, 0 replies; 10+ messages in thread From: Masahiro Yamada @ 2022-12-12 13:13 UTC (permalink / raw) To: Nathan Chancellor Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches, Alexander Lobakin On Thu, Dec 8, 2022 at 4:17 AM Nathan Chancellor <nathan@kernel.org> wrote: > > Commit 6c730bfc894f ("modpost: handle -ffunction-sections") added > ".text.*" to the OTHER_TEXT_SECTIONS macro to fix certain section > mismatch warnings. Unfortunately, this makes it impossible for modpost > to warn about section mismatchs with LTO, which implies "mismatches" Other than the typo, this patch looks good to me. It was just 1/2 was difficult for me to understand. > '-ffunction-sections', as all functions are put in their own > '.text.<func_name>' sections, which may still reference functions in > sections they are not supposed to, such as __init. > > Fix this by moving ".text.*" into TEXT_SECTIONS, so that configurations > with '-ffunction-sections' will see warnings about mismatched sections. > > Link: https://lore.kernel.org/Y39kI3MOtVI5BAnV@google.com/ > Reported-by: Vincent Donnefort <vdonnefort@google.com> > Reviewed-and-tested-by: Alexander Lobakin <alexandr.lobakin@intel.com> > Reviewed-by: Sami Tolvanen <samitolvanen@google.com> > Tested-by: Vincent Donnefort <vdonnefort@google.com> > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > scripts/mod/modpost.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 2c80da0220c3..c861beabc128 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -823,10 +823,10 @@ static void check_section(const char *modname, struct elf_info *elf, > #define ALL_EXIT_SECTIONS EXIT_SECTIONS, ALL_XXXEXIT_SECTIONS > > #define DATA_SECTIONS ".data", ".data.rel" > -#define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \ > +#define TEXT_SECTIONS ".text", ".text.*", ".sched.text", \ > ".kprobes.text", ".cpuidle.text", ".noinstr.text" > #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \ > - ".fixup", ".entry.text", ".exception.text", ".text.*", \ > + ".fixup", ".entry.text", ".exception.text", \ > ".coldtext", ".softirqentry.text" > > #define INIT_SECTIONS ".init.*" > -- > 2.38.1 > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-12-13 16:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-07 19:16 [PATCH v2 0/2] Fix lack of section mismatch warnings with LTO Nathan Chancellor 2022-12-07 19:16 ` [PATCH v2 1/2] padata: Mark padata_work_init() as __ref Nathan Chancellor 2022-12-07 19:56 ` Daniel Jordan 2022-12-12 13:07 ` Masahiro Yamada 2022-12-12 17:05 ` Nathan Chancellor 2022-12-12 19:21 ` Daniel Jordan 2022-12-12 20:06 ` Nathan Chancellor 2022-12-13 16:48 ` Nathan Chancellor 2022-12-07 19:16 ` [PATCH v2 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor 2022-12-12 13:13 ` Masahiro Yamada
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).