| Message ID | 20210705103806.2339467-1-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "mm/page_alloc: make should_fail_alloc_page() static" | expand |
On Mon, Jul 05, 2021 at 12:38:06PM +0200, Marco Elver wrote: > This reverts commit f7173090033c70886d925995e9dfdfb76dbb2441. > > Commit 76cd61739fd1 ("mm/error_inject: Fix allow_error_inject function > signatures") explicitly made should_fail_alloc_page() non-static, due to > worries of remaining compiler optimizations in the absence of function > side-effects while being noinline. > > Furthermore, kernel/bpf/verifier.c pushes should_fail_alloc_page onto > the btf_non_sleepable_error_inject BTF IDs set, which when enabling > CONFIG_DEBUG_INFO_BTF results in an error at the BTFIDS stage: > > FAILED unresolved symbol should_fail_alloc_page > > To avoid the W=1 warning, add a function declaration right above the > function itself, with a comment it is required in a BTF IDs set. > > Fixes: f7173090033c ("mm/page_alloc: make should_fail_alloc_page() static") > Cc: Mel Gorman <mgorman@techsingularity.net> > Cc: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Marco Elver <elver@google.com> Acked-by: Mel Gorman <mgorman@techsingularity.net> Out of curiousity though, why does block/blk-core.c not require something similar for should_fail_bio?
On Mon, Jul 05, 2021 at 12:37PM +0100, Mel Gorman wrote: > On Mon, Jul 05, 2021 at 12:38:06PM +0200, Marco Elver wrote: > > This reverts commit f7173090033c70886d925995e9dfdfb76dbb2441. > > > > Commit 76cd61739fd1 ("mm/error_inject: Fix allow_error_inject function > > signatures") explicitly made should_fail_alloc_page() non-static, due to > > worries of remaining compiler optimizations in the absence of function > > side-effects while being noinline. > > > > Furthermore, kernel/bpf/verifier.c pushes should_fail_alloc_page onto > > the btf_non_sleepable_error_inject BTF IDs set, which when enabling > > CONFIG_DEBUG_INFO_BTF results in an error at the BTFIDS stage: > > > > FAILED unresolved symbol should_fail_alloc_page > > > > To avoid the W=1 warning, add a function declaration right above the > > function itself, with a comment it is required in a BTF IDs set. > > > > Fixes: f7173090033c ("mm/page_alloc: make should_fail_alloc_page() static") > > Cc: Mel Gorman <mgorman@techsingularity.net> > > Cc: Alexei Starovoitov <ast@kernel.org> > > Signed-off-by: Marco Elver <elver@google.com> > > Acked-by: Mel Gorman <mgorman@techsingularity.net> > > Out of curiousity though, why does block/blk-core.c not require > something similar for should_fail_bio? It seems kernel/bpf/verifier.c doesn't refer to it in an BTF IDs set. Looks like should_fail_alloc_page is special for BPF purposes. I'm not a BPF maintainer, so hopefully someone can explain why should_fail_alloc_page is special for BPF. Thanks, -- Marco
On Mon, Jul 05, 2021 at 12:38:06PM +0200, Marco Elver wrote: > This reverts commit f7173090033c70886d925995e9dfdfb76dbb2441. > > Commit 76cd61739fd1 ("mm/error_inject: Fix allow_error_inject function > signatures") explicitly made should_fail_alloc_page() non-static, due to > worries of remaining compiler optimizations in the absence of function > side-effects while being noinline. > > Furthermore, kernel/bpf/verifier.c pushes should_fail_alloc_page onto > the btf_non_sleepable_error_inject BTF IDs set, which when enabling > CONFIG_DEBUG_INFO_BTF results in an error at the BTFIDS stage: > > FAILED unresolved symbol should_fail_alloc_page > > To avoid the W=1 warning, add a function declaration right above the > function itself, with a comment it is required in a BTF IDs set. NAK. We're not going to make symbols pointlessly global for broken instrumentation coe. Someone needs to fixthis eBPF mess as we had the same kind of issue before already.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index d6e94cc8066c..16e71d48d84e 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3831,7 +3831,13 @@ static inline bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) #endif /* CONFIG_FAIL_PAGE_ALLOC */ -static noinline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) +/* + * should_fail_alloc_page() is only called by page_alloc.c, however, is also + * included in a BTF IDs set and must remain non-static. Declare it to avoid a + * "missing prototypes" warning, and make it clear this is intentional. + */ +bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order); +noinline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) { return __should_fail_alloc_page(gfp_mask, order); }
This reverts commit f7173090033c70886d925995e9dfdfb76dbb2441. Commit 76cd61739fd1 ("mm/error_inject: Fix allow_error_inject function signatures") explicitly made should_fail_alloc_page() non-static, due to worries of remaining compiler optimizations in the absence of function side-effects while being noinline. Furthermore, kernel/bpf/verifier.c pushes should_fail_alloc_page onto the btf_non_sleepable_error_inject BTF IDs set, which when enabling CONFIG_DEBUG_INFO_BTF results in an error at the BTFIDS stage: FAILED unresolved symbol should_fail_alloc_page To avoid the W=1 warning, add a function declaration right above the function itself, with a comment it is required in a BTF IDs set. Fixes: f7173090033c ("mm/page_alloc: make should_fail_alloc_page() static") Cc: Mel Gorman <mgorman@techsingularity.net> Cc: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Marco Elver <elver@google.com> --- mm/page_alloc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)