On Fri, Nov 12, 2021 at 1:47 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Sat, Nov 13, 2021 at 03:04:11AM IST, Alexei Starovoitov wrote:
> > On Sat, Nov 13, 2021 at 01:54:21AM +0530, Kumar Kartikeya Dwivedi wrote:
> > >
> > > diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> > > index 7b73f97b1fa1..558479c13c77 100644
> > > --- a/tools/lib/bpf/gen_loader.c
> > > +++ b/tools/lib/bpf/gen_loader.c
> > > @@ -18,7 +18,7 @@
> > > #define MAX_USED_MAPS 64
> > > #define MAX_USED_PROGS 32
> > > #define MAX_KFUNC_DESCS 256
> > > -#define MAX_FD_ARRAY_SZ (MAX_USED_PROGS + MAX_KFUNC_DESCS)
> > > +#define MAX_FD_ARRAY_SZ (MAX_USED_MAPS + MAX_KFUNC_DESCS)
> >
> > Lol. Not sure how I missed it during code review :)
> >
> > > void bpf_gen__init(struct bpf_gen *gen, int log_level)
> > > {
> > > size_t stack_sz = sizeof(struct loader_stack);
> > > @@ -120,8 +146,12 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
> > >
> > > /* jump over cleanup code */
> > > emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0,
> > > - /* size of cleanup code below */
> > > - (stack_sz / 4) * 3 + 2));
> > > + /* size of cleanup code below (including map fd cleanup) */
> > > + (stack_sz / 4) * 3 + 2 + (MAX_USED_MAPS *
> > > + /* 6 insns for emit_sys_close_blob,
> > > + * 6 insns for debug_regs in emit_sys_close_blob
> > > + */
> > > + (6 + (gen->log_level ? 6 : 0)))));
> > >
> > > /* remember the label where all error branches will jump to */
> > > gen->cleanup_label = gen->insn_cur - gen->insn_start;
> > > @@ -131,37 +161,19 @@ void bpf_gen__init(struct bpf_gen *gen, int log_level)
> > > emit(gen, BPF_JMP_IMM(BPF_JSLE, BPF_REG_1, 0, 1));
> > > emit(gen, BPF_EMIT_CALL(BPF_FUNC_sys_close));
> > > }
> > > + gen->fd_array = add_data(gen, NULL, MAX_FD_ARRAY_SZ * sizeof(int));
> >
> > could you move this line to be the first thing in bpf_gen__init() ?
> > Otherwise it looks like that fd_array is only used in cleanup part while
> > it's actually needed everywhere.
> >
>
> Ack. Also thinking of not reordering add_data as that pollutes git blame, but
> just adding a declaration before use.
git blame is not a big deal. Both options are fine.
>
> > > + for (i = 0; i < MAX_USED_MAPS; i++)
> > > + emit_sys_close_blob(gen, blob_fd_array_off(gen, i));
> >
> > I confess that commit 30f51aedabda ("libbpf: Cleanup temp FDs when intermediate sys_bpf fails.")
> > wasn't great in terms of redundant code gen for closing all 32 + 64 FDs.
> > But can we make it better while we're at it?
> > Most bpf files don't have 32 progs and 64 maps while gen_loader emits
> > (32 + 64) * 6 = 576 instructions (without debug).
> > While debugging/developing gen_loader this large cleanup code is just noise.
> >
>
> Yeah, I've been thinking about this for a while, there's also lots of similar
> code gen in e.g. test_ksyms_module.o for the relocations. It might make sense to
> move to subprog approach and emit a BPF_CALL, but that's a separate issue. I can
> look into that too if it sounds good (but maybe you already tried this and ran
> into issues).
Do you mean to split things like emit_sys_close into its own subprog
and call it ?
I'm not sure it's large enough to do that.
Or you mean the code that copies?
There is a lot of it in test_ksyms_module.o
Would be good to reduce it, but it's a corner case
and probably not typical.
I'm all ears.
> > So cleanup code can close only nr_progs + nr_maps FDs.
> > gen_loader prog will be much shorter and will be processed by the verifier faster.
> > MAX_FD_ARRAY_SZ can stay fixed. Reducing data size is not worth it.
> > wdyt?
>
> Looks nice, I'll rework it like this.
Thanks. Pls keep targeting bpf tree with respin.
