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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Aug 2, 2022 at 9:48 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Mon, 2022-08-01 at 18:49 -0700, Alexei Starovoitov wrote:
> > On Mon, Aug 1, 2022 at 4:27 PM Kui-Feng Lee <kuifeng@fb.com> 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            |  4 ++
> > >  include/uapi/linux/bpf.h       | 23 +++++++++
> > >  kernel/bpf/task_iter.c         | 93 ++++++++++++++++++++++++++----
> > > ----
> > >  tools/include/uapi/linux/bpf.h | 23 +++++++++
> > >  4 files changed, 121 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 11950029284f..3c26dbfc9cef 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -1718,6 +1718,10 @@ int bpf_obj_get_user(const char __user
> > > *pathname, int flags);
> > >
> > >  struct bpf_iter_aux_info {
> > >         struct bpf_map *map;
> > > +       struct {
> > > +               u32     tid;
> > > +               u8      type;
> > > +       } task;
> > >  };
> > >
> > >  typedef int (*bpf_iter_attach_target_t)(struct bpf_prog *prog,
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index ffcbf79a556b..ed5ba501609f 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -87,10 +87,33 @@ struct bpf_cgroup_storage_key {
> > >         __u32   attach_type;            /* program attach type
> > > (enum bpf_attach_type) */
> > >  };
> > >
> > > +enum bpf_task_iter_type {
> > > +       BPF_TASK_ITER_ALL = 0,
> > > +       BPF_TASK_ITER_TID,
> > > +};
> > > +
> > >  union bpf_iter_link_info {
> > >         struct {
> > >                 __u32   map_fd;
> > >         } map;
> > > +       /*
> > > +        * Parameters of task iterators.
> > > +        */
> > > +       struct {
> > > +               __u32   pid_fd;
> > > +               /*
> > > +                * The type of the iterator.
> > > +                *
> > > +                * It can be one of enum bpf_task_iter_type.
> > > +                *
> > > +                * BPF_TASK_ITER_ALL (default)
> > > +                *      The iterator iterates over resources of
> > > everyprocess.
> > > +                *
> > > +                * BPF_TASK_ITER_TID
> > > +                *      You should also set *pid_fd* to iterate
> > > over one task.
> > > +                */
> > > +               __u8    type;   /* BPF_TASK_ITER_* */
> >
> > __u8 might be a pain for future extensibility.
>
> Do you mean the problem caused by padding?

Not Alexei, but I agree that there is no reason to try to save a few
bytes here. Let's use u32 or just plain 32-bit enum. Please also put
it in front of pid_fd (first field in this substruct), so that it's
easier to extend this with more information about "iteration target"
(e.g., if we later want to iterate tasks within cgroup, we might end
up specifying cgroup_id, which I believe is 64-bit, so it would be
nice to be able to just do a union across {pid_fd, pid, cgroup_fd,
cgroup_id}.

>
> > big vs little endian will be another potential issue.
>
> Do we need binary compatible for different platforms?
> I don't get the point of endian.  Could you explain it more?
>
> >
> > Maybe use enum bpf_task_iter_type type; here and
> > move the comment to enum def ?
> > Or rename it to '__u32 flags;' ?
>