On Tue, Sep 6, 2022 at 2:17 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> On 02/09/2022 11:23, weiyongjun (A) wrote:
> > Hi Quentin,
> >
> > On 2022/8/26 18:45, Quentin Monnet wrote:
> >> Hi Andrii,
> >>
> >> On 25/08/2022 19:37, Andrii Nakryiko wrote:
> >>> On Thu, Aug 25, 2022 at 8:28 AM Quentin Monnet
> >>> <quentin@xxxxxxxxxxxxx> wrote:
> >>>>
> >>>> Hi Wei,
> >>>>
> >>>> Apologies for failing to answer to your previous email and for the
> >>>> delay
> >>>> on this one, I just found out GMail had classified them as spam :(.
> >>>>
> >>>> So as for your last message, yes: your understanding of my previous
> >>>> answer was correct. Thanks for the patch below! Some comments inline.
> >>>>
> >>>
> >>> Do we really want to add such a specific command to bpftool that would
> >>> attach BPF object files with programs of only RAW_TRACEPOINT and
> >>> RAW_TRACEPOINT_WRITABLE type?
> >>>
> >>> I could understand if we added something that would be equivalent of
> >>> BPF skeleton's auto-attach method. That would make sense in some
> >>> contexts, especially for some quick testing and validation, to avoid
> >>> writing (a rather simple) user-space loading code.
> >>
> >> Do you mean loading and attaching in a single step, or keeping the
> >> possibility to load first as in the current proposal?
> >>
> >>>
> >>> But "perf attach" for raw_tp programs only? Seem way too limited and
> >>> specific, just adding bloat to bpftool, IMO.
> >>
> >> We already support attaching some kinds of program types through
> >> "prog|cgroup|net attach". Here I thought we could add support for other
> >> types as a follow-up, but thinking again, you're probably right, it
> >> would be best if all the types were supported from the start. Wei, have
> >> you looked into how much work it would be to add support for
> >> tracepoints, k(ret)probes, u(ret)probes as well? The code should be
> >> mostly identical?
> >>
> >
> >
> > When I try to add others support, I found that we need to dup many code
> > with libbpf has already done, since we lost the section name info.
I don't think bpftool should be parsing SEC() definitions. As Quentin
suggested, just bpf_program__attach() should be enough.
>
> What amount of code does this represent? Do you have a version of the
> patch accessible somewhere? I trust you, I'm just curious about the
> steps we're missing without having processed the section name info, but
> I can go and look at the details myself otherwise.
>
> > I have tried to add auto-attach, it seems more easier then perf
> > attach command.
> >
> > What's about your opinion?
>
> Yes I'm good with that approach, too.
>
> > Maybe we only need a little of changes like this:
> >
> > $ bpftool prog load test.o /sys/fs/bpf/test auto-attach
>
> "auto_attach", other keywords use underscores rather than dashes.
>
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index c81362a001ba..87fab89eaa07 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -1464,6 +1464,7 @@ static int load_with_options(int argc, char
>
>
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 3ad139285fad..915ec0a97583 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -7773,15 +7773,32 @@ int bpf_program__pin(struct bpf_program *prog,
> > const char *path)
> > if (err)
> > return libbpf_err(err);
> >
> > - if (bpf_obj_pin(prog->fd, path)) {
> > - err = -errno;
> > - cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> > - pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name,
> > path, cp);
> > - return libbpf_err(err);
> > + if (prog->autoattach) {
> > + struct bpf_link *link;
> > +
> > + link = bpf_program__attach(prog);
> > + err = libbpf_get_error(link);
> > + if (err)
> > + goto err_out;
> > +
> > + err = bpf_link__pin(link, path);
> > + if (err) {
> > + bpf_link__destroy(link);
> > + goto err_out;
> > + }
> > + } else {
> > + if (bpf_obj_pin(prog->fd, path)) {
> > + err = -errno;
> > + goto err_out;
> > + }
> > }
> >
> > pr_debug("prog '%s': pinned at '%s'\n", prog->name, path);
> > return 0;
> > +err_out:
> > + cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> > + pr_warn("prog '%s': failed to pin at '%s': %s\n", prog->name, path,
> > cp);
> > + return libbpf_err(err);
> > }
> >
> > int bpf_program__unpin(struct bpf_program *prog, const char *path)
>
> I don't think it is correct to modify libbpf's bpf_program__pin()
> though, because it shouldn't be its role to attach and also because I
> think it might lead to a second attempt to attach if the user tries to
> pin in addition to running the auto-attach from a skeleton. Let's just
> call bpf_program__attach() from bpftool instead?
+1, libbpf shouldn't be modified for this feature
