On Fri, Dec 3, 2021 at 12:11 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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.
Ok, fair enough. I've renamed specs to specs_scratch to make this more
explicit and pushed to bpf-next.
