Re: [PATCH bpf-next v6] bpftool: Add bpf_cookie to link output

[Quentin Monnet <quentin@isovalent.com>]

2022-03-26 10:08 UTC+0100 ~ Dmitry Dolgov <9erthalion6@gmail.com>
>> On Sat, Mar 26, 2022 at 01:38:36AM +0000, Quentin Monnet wrote:
>> 2022-03-09 17:31 UTC+0100 ~ Dmitrii Dolgov <9erthalion6@gmail.com>
>>> Commit 82e6b1eee6a8 ("bpf: Allow to specify user-provided bpf_cookie for
>>> BPF perf links") introduced the concept of user specified bpf_cookie,
>>> which could be accessed by BPF programs using bpf_get_attach_cookie().
>>> For troubleshooting purposes it is convenient to expose bpf_cookie via
>>> bpftool as well, so there is no need to meddle with the target BPF
>>> program itself.
>>>
>>> [...]
>>>
>>> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
>>> index 7c384d10e95f..bb6c969a114a 100644
>>> --- a/tools/bpf/bpftool/pids.c
>>> +++ b/tools/bpf/bpftool/pids.c
>>> @@ -78,6 +78,8 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
>>>  	ref->pid = e->pid;
>>>  	memcpy(ref->comm, e->comm, sizeof(ref->comm));
>>>  	refs->ref_cnt = 1;
>>> +	refs->has_bpf_cookie = e->has_bpf_cookie;
>>> +	refs->bpf_cookie = e->bpf_cookie;
>>>
>>>  	err = hashmap__append(map, u32_as_hash_field(e->id), refs);
>>>  	if (err)
>>> @@ -205,6 +207,9 @@ void emit_obj_refs_json(struct hashmap *map, __u32 id,
>>>  		if (refs->ref_cnt == 0)
>>>  			break;
>>>
>>> +		if (refs->has_bpf_cookie)
>>> +			jsonw_lluint_field(json_writer, "bpf_cookie", refs->bpf_cookie);
>>> +
>>
>> Thinking again about this patch, shouldn't the JSON output for the
>> cookie(s) be an array if we expect to have several cookies for
>> multi-attach links in the future?
> 
> Interesting point. My impression is that this could be done together
> with the other changes about making multi-attach links possible (I
> didn't miss anything, it's not yet implemented, right?). On the other
> hand I'm planning to prepare few more patches in similar direction -- so
> if everyone agrees it has to be extended to an array now, I can tackle
> this as well.

Correct, it's not implemented yet for multi-attach links. My concern
here is to avoid changing the JSON structure in the future (to avoid
breaking changes for tools that would process the JSON). If we know
we're likely to have several cookies in the future, it may be worth
using an array “from start” (since no version has been tagged yet after
you added support for the cookie).

Quentin