On Tue, Nov 16, 2021 at 07:45:47PM -0800, Andrii Nakryiko wrote:
> On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > Without lskel the CO-RE relocations are processed by libbpf before any other
> > work is done. Instead, when lksel is needed, remember relocation as RELO_CORE
>
> typo: lskel
>
> > kind. Then when loader prog is generated for a given bpf program pass CO-RE
> > relos of that program to gen loader via bpf_gen__record_relo_core(). The gen
> > loader will remember them as-is and pass it later as-is into the kernel.
> >
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/bpf_gen_internal.h | 3 +
> > tools/lib/bpf/gen_loader.c | 41 +++++++++++-
> > tools/lib/bpf/libbpf.c | 104 +++++++++++++++++++++++--------
> > 3 files changed, 119 insertions(+), 29 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
> > index 75ca9fb857b2..ed162fdeecf6 100644
> > --- a/tools/lib/bpf/bpf_gen_internal.h
> > +++ b/tools/lib/bpf/bpf_gen_internal.h
> > @@ -39,6 +39,8 @@ struct bpf_gen {
> > int error;
> > struct ksym_relo_desc *relos;
> > int relo_cnt;
> > + struct bpf_core_relo *core_relo;
>
> this is named as a singular pointer to one relocation, core_relos
> would be a more natural name for an array?
I had it with "s" at the beginning, but it was forcing core_relo_cnt variable
to be called core_relos_cnt to be consistent.
And later it spills this "consistency" into uapi core_relos_cnt in bpf_attr.
But here it conflicts with line_info_cnt and func_info_cnt.
Once I realized that I went back and got rid of this "s".
>
> > + int core_relo_cnt;
> > char attach_target[128];
> > int attach_kind;
> > struct ksym_desc *ksyms;
> > @@ -61,5 +63,6 @@ void bpf_gen__map_freeze(struct bpf_gen *gen, int map_idx);
> > void bpf_gen__record_attach_target(struct bpf_gen *gen, const char *name, enum bpf_attach_type type);
> > void bpf_gen__record_extern(struct bpf_gen *gen, const char *name, bool is_weak,
> > bool is_typeless, int kind, int insn_idx);
>
> [...]
>
> > @@ -6581,6 +6623,16 @@ static int bpf_program__record_externs(struct bpf_program *prog)
> > ext->is_weak, false, BTF_KIND_FUNC,
> > relo->insn_idx);
> > break;
> > + case RELO_CORE: {
>
> This is not an extern, it doesn't make sense to have it here. But I
> also don't understand why we need to add RELO_CORE and extend struct
> relo_desc in the first place, just to pass it as bpf_core_relo into
> gen_loader. Why can't gen_loader just record this directly at the
> place of record_relo_core() call?
Sorry. I should have explained it in commit log.
The normal libbpf flow is to process CO-RE early before call relos happen.
In case of gen_loader the core relos have to be added to other relos to be
copied together when bpf static function is appended in different places to
other main bpf progs.
During the copy the append_subprog_relos() will adjust insn_idx for
normal relos and for RELO_CORE kind too.
When that is done each struct reloc_desc has good relos for specific main prog.
Just noticed that 'insn_idx += prog->sub_insn_off;' in this patch is redundant.
That was a left over of earlier debugging.
Also in bpf_object__relocate_data() the comment:
case RELO_CORE:
/* handled already */
break;
is not correct either.
It should read "will be handled by bpf_program__record_externs() later".
Just before bpf_object_load_prog_instance().
