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

[Kui-Feng Lee <kuifeng@fb.com>]

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?

> 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;' ?