From: Kui-Feng Lee <kuifeng@fb.com>
To: "andrii.nakryiko@gmail.com" <andrii.nakryiko@gmail.com>
Cc: "daniel@iogearbox.net" <daniel@iogearbox.net>,
Kernel Team <Kernel-team@fb.com>, Yonghong Song <yhs@fb.com>,
"ast@kernel.org" <ast@kernel.org>,
"andrii@kernel.org" <andrii@kernel.org>,
"bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v6 1/4] bpf: Parameterize task iterators.
Date: Thu, 25 Aug 2022 00:16:35 +0000 [thread overview]
Message-ID: <52171bf63f54b311116988cefd275c0847396d45.camel@fb.com> (raw)
In-Reply-To: <CAEf4Bzai7s1E6Y5=+URKXvSO7h8NJ6aNLxZCQrTq2ucTUp=S_Q@mail.gmail.com>
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?
>
> > 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;
> >
> > @@ -73,7 +90,7 @@ static void *task_seq_next(struct seq_file *seq,
> > void *v, loff_t *pos)
> > ++*pos;
> > ++info->tid;
> > put_task_struct((struct task_struct *)v);
> > - 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;
> >
> > @@ -117,6 +134,48 @@ static void task_seq_stop(struct seq_file
> > *seq, void *v)
> > put_task_struct((struct task_struct *)v);
> > }
> >
> > +static int bpf_iter_attach_task(struct bpf_prog *prog,
> > + union bpf_iter_link_info *linfo,
> > + struct bpf_iter_aux_info *aux)
> > +{
> > + unsigned int flags;
> > + struct pid_namespace *ns;
> > + struct pid *pid;
> > + pid_t tgid;
>
> it seems it would be simpler to first check that at most one of
> tid/pid/pid_fd is set instead of making sure that aux->task.type
> wasn't already set.
>
> How about
>
> if (!!linfo->task.tid + !!linfo->task.pid + !!linfo->task.pid_fd > 1)
> return -EINVAL;
>
> ?
Agree
>
> > +
> > + aux->task.type = BPF_TASK_ITER_ALL;
> > + if (linfo->task.tid != 0) {
> > + aux->task.type = BPF_TASK_ITER_TID;
> > + aux->task.pid = linfo->task.tid;
> > + }
> > + if (linfo->task.pid != 0) {
> > + if (aux->task.type != BPF_TASK_ITER_ALL)
> > + return -EINVAL;
> > +
> > + aux->task.type = BPF_TASK_ITER_TGID;
> > + aux->task.pid = linfo->task.pid;
> > + }
> > + if (linfo->task.pid_fd != 0) {
> > + if (aux->task.type != BPF_TASK_ITER_ALL)
> > + return -EINVAL;
> > +
> > + aux->task.type = BPF_TASK_ITER_TGID;
> > + ns = task_active_pid_ns(current);
> > + if (IS_ERR(ns))
> > + return PTR_ERR(ns);
> > +
> > + pid = pidfd_get_pid(linfo->task.pid_fd, &flags);
> > + if (IS_ERR(pid))
> > + return PTR_ERR(pid);
> > +
> > + tgid = pid_nr_ns(pid, ns);
> > + aux->task.pid = tgid;
> > + put_pid(pid);
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> [...]
next prev parent reply other threads:[~2022-08-25 0:16 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-19 22:09 [PATCH bpf-next v6 0/4] Parameterize task iterators Kui-Feng Lee
2022-08-19 22:09 ` [PATCH bpf-next v6 1/4] bpf: " Kui-Feng Lee
2022-08-24 19:33 ` Yonghong Song
2022-08-24 22:20 ` Andrii Nakryiko
2022-08-25 0:16 ` Kui-Feng Lee [this message]
2022-08-25 21:13 ` Andrii Nakryiko
2022-08-26 14:49 ` Kui-Feng Lee
2022-08-27 4:50 ` Andrii Nakryiko
2022-08-19 22:09 ` [PATCH bpf-next v6 2/4] bpf: Handle bpf_link_info for the parameterized task BPF iterators Kui-Feng Lee
2022-08-24 19:41 ` Yonghong Song
2022-08-19 22:09 ` [PATCH bpf-next v6 3/4] bpf: Handle show_fdinfo " Kui-Feng Lee
2022-08-24 19:49 ` Yonghong Song
2022-08-19 22:09 ` [PATCH bpf-next v6 4/4] selftests/bpf: Test " Kui-Feng Lee
2022-08-24 20:50 ` Yonghong Song
2022-08-24 22:30 ` Andrii Nakryiko
2022-08-25 1:07 ` Kui-Feng Lee
2022-08-24 22:06 ` [PATCH bpf-next v6 0/4] Parameterize task iterators Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52171bf63f54b311116988cefd275c0847396d45.camel@fb.com \
--to=kuifeng@fb.com \
--cc=Kernel-team@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).