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 <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
---
mm/page_alloc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
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);
}
--
2.32.0.93.g670b81a890-goog
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 <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Signed-off-by: Marco Elver <[email protected]>
Acked-by: Mel Gorman <[email protected]>
Out of curiousity though, why does block/blk-core.c not require
something similar for should_fail_bio?
--
Mel Gorman
SUSE Labs
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 <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Signed-off-by: Marco Elver <[email protected]>
>
> Acked-by: Mel Gorman <[email protected]>
>
> 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.