Re: [PATCH bpf-next v5 1/3] bpf: Parameterize task iterators.

[Yonghong Song <yhs@fb.com>]

On 8/15/22 9:42 PM, Andrii Nakryiko wrote:
> On Sat, Aug 13, 2022 at 3:17 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 8/10/22 5:16 PM, Kui-Feng Lee wrote:
>>> Allow creating an iterator that loops through resources of one task/thread.
>>>
>>> People could only create iterators to loop through all resources of
>>> files, vma, and tasks in the system, even though they were interested
>>> in only the resources of a specific task or process.  Passing the
>>> additional parameters, people can now create an iterator to go
>>> through all resources or only the resources of a task.
>>>
>>> Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
>>> ---
>>>    include/linux/bpf.h            |  29 ++++++++
>>>    include/uapi/linux/bpf.h       |   8 +++
>>>    kernel/bpf/task_iter.c         | 126 ++++++++++++++++++++++++++-------
>>>    tools/include/uapi/linux/bpf.h |   8 +++
>>>    4 files changed, 147 insertions(+), 24 deletions(-)
>>>
> 
> [...]
> 
>>> +     struct {
>>> +             __u32   tid;
>>> +             __u32   tgid;
>>> +             __u32   pid_fd;
>>
>> The above is a max of kernel and user space terminologies.
>> tid/pid are user space concept and tgid is a kernel space
>> concept.
>>
>> In bpf uapi header, we have
>>
>> struct bpf_pidns_info {
>>           __u32 pid;
>>           __u32 tgid;
>> };
>>
>> which uses kernel terminologies.
>>
>> So I suggest the bpf_iter_link_info.task can also
>> use pure kernel terminology pid/tgid/tgid_fd here.
> 
> Except tgid_fd is a made up terminology. It is called pidfd in
> documentation and even if pidfd gains add support for threads (tasks),
> it would still be created through pidfd_open() syscall, probably. So
> it seems better to stick to "pidfd" here.
> 
> As for pid/tgid precedent. Are we referring to
> bpf_get_current_pid_tgid() BPF helper and struct bpf_pidns_info? Those
> are kernel-side BPF helper and kernel-side auxiliary type to describe
> return results of another in-kernel helper, so I think it's a bit less
> relevant here to set a precedent.
> 
> On the other hand, if we look at user-space-facing perf_event
> subsystem UAPI, for example, it seems to be using pid/tid terminology
> (and so does getpid()/gettid() syscall, etc). So I wonder if for a
> user-space-facing API it's better to stick with user-space-facing
> terminology?

I don't have strong preferences here as long as all terminologies are
consistent. user-space-facing API is okay.

Currently, we only have pid_fd to traverse all tasks in a process.
Based on an early discussion, it is possible that
pidfd_open syscall might adapt to return a fd for a task
in the future if necessary.
So we might have tid_fd as well to traverse a single task.

> 
>>
>> Alternative, using pure user space terminology
>> can be tid/pid/pid_fd but seems the kernel terminology
>> might be better since we already have precedence.
>>
>>
>>> +     } task;
>>>    };
>>>
> 
> [...]