* [PATCH bpf-next] bpf: fix bpf_iter's task iterator logic
@ 2020-05-13 21:20 Andrii Nakryiko
2020-05-13 22:11 ` Yonghong Song
2020-05-13 22:42 ` Alexei Starovoitov
0 siblings, 2 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2020-05-13 21:20 UTC (permalink / raw)
To: bpf, netdev, ast, daniel
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Yonghong Song
task_seq_get_next might stop prematurely if get_pid_task() fails to get
task_struct. Failure to do so doesn't mean that there are no more tasks with
higher pids. Procfs's iteration algorithm (see next_tgid in fs/proc/base.c)
does a retry in such case. After this fix, instead of stopping prematurely
after about 300 tasks on my server, bpf_iter program now returns >4000, which
sounds much closer to reality.
Cc: Yonghong Song <yhs@fb.com>
Fixes: eaaacd23910f ("bpf: Add task and task/file iterator targets")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
kernel/bpf/task_iter.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index a9b7264dda08..e1836def6738 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -27,9 +27,15 @@ static struct task_struct *task_seq_get_next(struct pid_namespace *ns,
struct pid *pid;
rcu_read_lock();
+retry:
pid = idr_get_next(&ns->idr, tid);
- if (pid)
+ if (pid) {
task = get_pid_task(pid, PIDTYPE_PID);
+ if (!task) {
+ *tid++;
+ goto retry;
+ }
+ }
rcu_read_unlock();
return task;
--
2.24.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] bpf: fix bpf_iter's task iterator logic
2020-05-13 21:20 [PATCH bpf-next] bpf: fix bpf_iter's task iterator logic Andrii Nakryiko
@ 2020-05-13 22:11 ` Yonghong Song
2020-05-13 22:42 ` Alexei Starovoitov
1 sibling, 0 replies; 4+ messages in thread
From: Yonghong Song @ 2020-05-13 22:11 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team
On 5/13/20 2:20 PM, Andrii Nakryiko wrote:
> task_seq_get_next might stop prematurely if get_pid_task() fails to get
> task_struct. Failure to do so doesn't mean that there are no more tasks with
> higher pids. Procfs's iteration algorithm (see next_tgid in fs/proc/base.c)
> does a retry in such case. After this fix, instead of stopping prematurely
> after about 300 tasks on my server, bpf_iter program now returns >4000, which
> sounds much closer to reality.
>
> Cc: Yonghong Song <yhs@fb.com>
> Fixes: eaaacd23910f ("bpf: Add task and task/file iterator targets")
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Thanks for the fix. We did this retry logic for bpf_map which is
idr based logic too. But forgot to check for task which has the
same issue.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> kernel/bpf/task_iter.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index a9b7264dda08..e1836def6738 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
> @@ -27,9 +27,15 @@ static struct task_struct *task_seq_get_next(struct pid_namespace *ns,
> struct pid *pid;
>
> rcu_read_lock();
> +retry:
> pid = idr_get_next(&ns->idr, tid);
> - if (pid)
> + if (pid) {
> task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task) {
> + *tid++;
> + goto retry;
> + }
> + }
> rcu_read_unlock();
>
> return task;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] bpf: fix bpf_iter's task iterator logic
2020-05-13 21:20 [PATCH bpf-next] bpf: fix bpf_iter's task iterator logic Andrii Nakryiko
2020-05-13 22:11 ` Yonghong Song
@ 2020-05-13 22:42 ` Alexei Starovoitov
2020-05-14 5:45 ` Andrii Nakryiko
1 sibling, 1 reply; 4+ messages in thread
From: Alexei Starovoitov @ 2020-05-13 22:42 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Kernel Team, Yonghong Song
On Wed, May 13, 2020 at 2:23 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> task_seq_get_next might stop prematurely if get_pid_task() fails to get
> task_struct. Failure to do so doesn't mean that there are no more tasks with
> higher pids. Procfs's iteration algorithm (see next_tgid in fs/proc/base.c)
> does a retry in such case. After this fix, instead of stopping prematurely
> after about 300 tasks on my server, bpf_iter program now returns >4000, which
> sounds much closer to reality.
>
> Cc: Yonghong Song <yhs@fb.com>
> Fixes: eaaacd23910f ("bpf: Add task and task/file iterator targets")
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> kernel/bpf/task_iter.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index a9b7264dda08..e1836def6738 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
> @@ -27,9 +27,15 @@ static struct task_struct *task_seq_get_next(struct pid_namespace *ns,
> struct pid *pid;
>
> rcu_read_lock();
> +retry:
> pid = idr_get_next(&ns->idr, tid);
> - if (pid)
> + if (pid) {
> task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task) {
> + *tid++;
../kernel/bpf/task_iter.c: In function ‘task_seq_get_next’:
../kernel/bpf/task_iter.c:35:4: warning: value computed is not used
[-Wunused-value]
35 | *tid++;
| ^~~~~~
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] bpf: fix bpf_iter's task iterator logic
2020-05-13 22:42 ` Alexei Starovoitov
@ 2020-05-14 5:45 ` Andrii Nakryiko
0 siblings, 0 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2020-05-14 5:45 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, Network Development, Alexei Starovoitov,
Daniel Borkmann, Kernel Team, Yonghong Song
On Wed, May 13, 2020 at 3:42 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, May 13, 2020 at 2:23 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > task_seq_get_next might stop prematurely if get_pid_task() fails to get
> > task_struct. Failure to do so doesn't mean that there are no more tasks with
> > higher pids. Procfs's iteration algorithm (see next_tgid in fs/proc/base.c)
> > does a retry in such case. After this fix, instead of stopping prematurely
> > after about 300 tasks on my server, bpf_iter program now returns >4000, which
> > sounds much closer to reality.
> >
> > Cc: Yonghong Song <yhs@fb.com>
> > Fixes: eaaacd23910f ("bpf: Add task and task/file iterator targets")
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > kernel/bpf/task_iter.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> > index a9b7264dda08..e1836def6738 100644
> > --- a/kernel/bpf/task_iter.c
> > +++ b/kernel/bpf/task_iter.c
> > @@ -27,9 +27,15 @@ static struct task_struct *task_seq_get_next(struct pid_namespace *ns,
> > struct pid *pid;
> >
> > rcu_read_lock();
> > +retry:
> > pid = idr_get_next(&ns->idr, tid);
> > - if (pid)
> > + if (pid) {
> > task = get_pid_task(pid, PIDTYPE_PID);
> > + if (!task) {
> > + *tid++;
>
> ../kernel/bpf/task_iter.c: In function ‘task_seq_get_next’:
> ../kernel/bpf/task_iter.c:35:4: warning: value computed is not used
> [-Wunused-value]
> 35 | *tid++;
> | ^~~~~~
welp... thanks, fixing to prefix form
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-14 5:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 21:20 [PATCH bpf-next] bpf: fix bpf_iter's task iterator logic Andrii Nakryiko
2020-05-13 22:11 ` Yonghong Song
2020-05-13 22:42 ` Alexei Starovoitov
2020-05-14 5:45 ` Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.