Re: [PATCH bpf-next v6 1/4] bpf: Parameterize task iterators.

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

On Thu, 2022-08-25 at 14:13 -0700, Andrii Nakryiko wrote:
> On Wed, Aug 24, 2022 at 5:16 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > On Wed, 2022-08-24 at 15:20 -0700, Andrii Nakryiko wrote:
> > > On Fri, Aug 19, 2022 at 3:09 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            |  25 +++++++
> > > >  include/uapi/linux/bpf.h       |   6 ++
> > > >  kernel/bpf/task_iter.c         | 116
> > > > ++++++++++++++++++++++++++---
> > > > ----
> > > >  tools/include/uapi/linux/bpf.h |   6 ++
> > > >  4 files changed, 129 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index 39bd36359c1e..59712dd917d8 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -1729,8 +1729,33 @@ int bpf_obj_get_user(const char __user
> > > > *pathname, int flags);
> > > >         extern int bpf_iter_ ## target(args);                  
> > > > \
> > > >         int __init bpf_iter_ ## target(args) { return 0; }
> > > > 
> > > > +/*
> > > > + * The task type of iterators.
> > > > + *
> > > > + * For BPF task iterators, they can be parameterized with
> > > > various
> > > > + * parameters to visit only some of tasks.
> > > > + *
> > > > + * BPF_TASK_ITER_ALL (default)
> > > > + *     Iterate over resources of every task.
> > > > + *
> > > > + * BPF_TASK_ITER_TID
> > > > + *     Iterate over resources of a task/tid.
> > > > + *
> > > > + * BPF_TASK_ITER_TGID
> > > > + *     Iterate over reosurces of evevry task of a process /
> > > > task
> > > > group.
> > > 
> > > typos: resources, every
> > > 
> > > > + */
> > > > +enum bpf_iter_task_type {
> > > > +       BPF_TASK_ITER_ALL = 0,
> > > > +       BPF_TASK_ITER_TID,
> > > > +       BPF_TASK_ITER_TGID,
> > > > +};
> > > > +
> > > 
> > > [...]
> > > 
> > > >         rcu_read_lock();
> > > >  retry:
> > > > -       pid = find_ge_pid(*tid, ns);
> > > > +       pid = find_ge_pid(*tid, common->ns);
> > > >         if (pid) {
> > > > -               *tid = pid_nr_ns(pid, ns);
> > > > +               *tid = pid_nr_ns(pid, common->ns);
> > > >                 task = get_pid_task(pid, PIDTYPE_PID);
> > > >                 if (!task) {
> > > >                         ++*tid;
> > > >                         goto retry;
> > > > -               } else if (skip_if_dup_files &&
> > > > !thread_group_leader(task) &&
> > > > -                          task->files == task->group_leader-
> > > > > files) {
> > > > +               } else if ((skip_if_dup_files &&
> > > > !thread_group_leader(task) &&
> > > > +                           task->files == task->group_leader-
> > > > > files) ||
> > > > +                          (common->type == BPF_TASK_ITER_TGID
> > > > &&
> > > > +                           __task_pid_nr_ns(task,
> > > > PIDTYPE_TGID,
> > > > common->ns) != common->pid)) {
> > > 
> > > it gets super hard to follow this logic, would a simple helper
> > > function to calculate this condition (and maybe some comments to
> > > explain the logic behind these checks?) make it a bit more
> > > readable?
> > 
> > !matched_task(task, common, skip_if_dup_file)?
> > 
> > bool matched_task(struct task_struct *task,
> >                   struct bpf_iter_seq_task_common *common,
> >                   bool skip_if_dup_file) {
> >         /* Should not have the same 'files' if skip_if_dup_file is
> > true
> > */
> >         bool diff_files_if =
> >                 !skip_if_dup_file ||
> >                 (thread_group_leader(task) &&
> >                 task->file != task->gorup_leader->fies);
> >         /* Should have the given tgid if the type is
> > BPF_TASK_ITER_TGI
> > */
> >         bool have_tgid_if =
> >                 common->type != BPF_TASK_ITER_TGID ||
> >                 __task_pid_nr_ns(task, PIDTYPE_TGID,
> >                 common->ns) == common->pid;
> >         return diff_files_if && have_tgid_if;
> > }
> > 
> > How about this?
> > 
> 
> Hm... "matched_task" doesn't mean much, tbh, so not really. I wanted
> to suggest having a separate helper just for your TGID check and call
> it something more meaningful like "task_belongs_to_tgid". Can't come
> up with a good name for existing dup_file check, so I'd probably keep
> it as is. But also seems like there is same_thread_group() helper in
> include/linux/sched/signal.h, so let's look if we can use it, it
> seems
> like it's just comparing signal pointers (probably quite faster than
> what you are doing right now).
> 
> But looking at this some more made me realize that even if we specify
> pid (tgid in kernel terms) we are still going to iterate through all
> the tasks, essentially. Is that right? So TGID mode isn't great for
> speeding up, we should point out to users that if they want to
> iterate
> files of the process, they probably want to use TID mode and set tid
> to pid to use the early termination condition in TID.
> 
> It wasn't obvious to me until I re-read this patch like 3 times and
> wrote three different replies here :)
> 
> But then I also went looking at what procfs doing for
> /proc/<pid/task/* dirs. It does seem like there are faster ways to
> iterate all threads of a process. See next_tid() which uses
> next_thread(), etc. Can you please check those and see if we can have
> faster in-process iteration?
> 

I haven't notice this message until now.
It looks like very promise.  I will add it to next version.

> 
> > > 
> > > >                         put_task_struct(task);
> > > >                         task = NULL;
> > > >                         ++*tid;
> > > > @@ -56,7 +73,7 @@ static void *task_seq_start(struct seq_file
> > > > *seq,
> > > > loff_t *pos)
> > > >         struct bpf_iter_seq_task_info *info = seq->private;
> > > >         struct task_struct *task;
> > > > 
> > > > -       task = task_seq_get_next(info->common.ns, &info->tid,
> > > > false);
> > > > +       task = task_seq_get_next(&info->common, &info->tid,
> > > > false);
> > > >         if (!task)
> > > >                 return NULL;
> > > > 
> 
> [...]