On Fri, Dec 3, 2021 at 12:08 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
I don't understand the suggestion.
There is nothing to remember. It could have been just raw bytes
of appropriate size. It's temp data.
Passing them as 3 different args would make an impression
that they're actually meaningful. They're not. It's a scratch space.
