On Fri, Dec 3, 2021 at 10:28 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> Reduce bpf_core_apply_relo_insn() stack usage and bump
> BPF_CORE_SPEC_MAX_LEN limit back to 64.
>
> Fixes: 29db4bea1d10 ("bpf: Prepare relo_core.c for kernel duty.")
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
Looks good except for the three separate specs passed as an array.
Let's do separate input args and it should be good to go. Thanks.
> kernel/bpf/btf.c | 11 ++++++-
> tools/lib/bpf/libbpf.c | 4 ++-
> tools/lib/bpf/relo_core.c | 60 +++++++++++----------------------------
> tools/lib/bpf/relo_core.h | 30 +++++++++++++++++++-
> 4 files changed, 59 insertions(+), 46 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ed4258cb0832..2a902a946f70 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6742,8 +6742,16 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
> {
> bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL;
> struct bpf_core_cand_list cands = {};
> + struct bpf_core_spec *specs;
> int err;
>
> + /* ~4k of temp memory necessary to convert LLVM spec like "0:1:0:5"
> + * into arrays of btf_ids of struct fields and array indices.
> + */
> + specs = kcalloc(3, sizeof(*specs), GFP_KERNEL);
> + if (!specs)
> + return -ENOMEM;
> +
> if (need_cands) {
> struct bpf_cand_cache *cc;
> int i;
> @@ -6779,8 +6787,9 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
> }
>
> err = bpf_core_apply_relo_insn((void *)ctx->log, insn, relo->insn_off / 8,
> - relo, relo_idx, ctx->btf, &cands);
> + relo, relo_idx, ctx->btf, &cands, specs);
> out:
> + kfree(specs);
> if (need_cands) {
> kfree(cands.cands);
> mutex_unlock(&cand_cache_mutex);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index de260c94e418..1ad070b19bb4 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5515,6 +5515,7 @@ static int bpf_core_apply_relo(struct bpf_program *prog,
> const struct btf *local_btf,
> struct hashmap *cand_cache)
> {
> + struct bpf_core_spec specs[3] = {};
so I get why single kcalloc() is good on the kernel side, but there is
no reason to do it here, please define three separate variables
> const void *type_key = u32_as_hash_key(relo->type_id);
> struct bpf_core_cand_list *cands = NULL;
> const char *prog_name = prog->name;
[...]
> static bool is_flex_arr(const struct btf *btf,
> const struct bpf_core_accessor *acc,
> const struct btf_array *arr)
> @@ -1200,9 +1173,10 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
> const struct bpf_core_relo *relo,
> int relo_idx,
> const struct btf *local_btf,
> - struct bpf_core_cand_list *cands)
> + struct bpf_core_cand_list *cands,
> + struct bpf_core_spec *specs)
same here, let's pass three separate arguments instead of having to
remember which array element corresponds to which (local vs cand vs
targ). It doesn't prevent kernel-side from using an array and just
passing pointers.
> {
> - struct bpf_core_spec local_spec, cand_spec, targ_spec = {};
> + struct bpf_core_spec *local_spec = &specs[0], *cand_spec = &specs[1], *targ_spec = &specs[2];
> struct bpf_core_relo_res cand_res, targ_res;
> const struct btf_type *local_type;
> const char *local_name;
[...]
