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.
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?
