Re: [RFC PATCH v4] bpftool: Add bpf_cookie to link output

[Yonghong Song <yhs@fb.com>]

On 3/3/22 8:20 AM, Dmitry Dolgov wrote:
>> On Tue, Mar 01, 2022 at 11:53:56PM -0800, Yonghong Song wrote:
>>
>>
>> On 2/25/22 7:28 AM, Dmitrii Dolgov wrote:
>>> 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.
>>
>> Do you still need RFC tag? It looks like we have a consensus
>> with this bpf_iter approach, right?
>>
>> Please also add "bpf-next" to the tag for clarity purpose.
> 
> Yeah, you're right, it seems there is no need for RFC tag any more. Will
> add "bpf-next" tag as well, thanks for the suggestion.
> 
>>> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
>>> index 7c384d10e95f..152502c2d6f9 100644
>>> --- a/tools/bpf/bpftool/pids.c
>>> +++ b/tools/bpf/bpftool/pids.c
>>> @@ -55,6 +55,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++;
>>> +		refs->bpf_cookie_set = e->bpf_cookie_set;
>>> +		refs->bpf_cookie = e->bpf_cookie;
>>
>> Do we need here? It is weird that we overwrite the bpf_cookie with every new
>> 'pid' reference.
>>
>> When you create a link, the cookie is fixed for that link. You could pin
>> that link in bpffs e.g., /sys/fs/bpf/link1 and other programs can then
>> get a reference to the link1, but they should still have the same cookie. Is
>> that right?
> 
> Right, I have the same understanding about a single fixed cookie per
> link. But in this particular case the implementation uses
> hashmap__for_each_key_entry (which is essentially a loop with a
> condition inside) and inside it returns as soon as the first entry was
> found. So I guess it will not override the cookie with every new
> reference, do I see it correct?

They are not return if pid is not the same.

Let us say the same link is used for pid1 and pid2.
The pid1 case will have refs->bpf_cookie[_set] set properly.
The pid2 case will trigger the above code, and since for the same
link, cookie is fixed, so the above code is not really needed.