| Message ID | 20230822120549.GA22091@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [V2] bpf: task_group_seq_get_next: cleanup the usage of get/put_task_struct | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
| bpf/vmtest-bpf-PR | fail | merge-conflict |
Hi Oleg, On 8/22/23 2:05 PM, Oleg Nesterov wrote: > get_pid_task() makes no sense, the code does put_task_struct() soon after. > Use find_task_by_pid_ns() instead of find_pid_ns + get_pid_task and kill > put_task_struct(), this allows to do get_task_struct() only once before > return. > > While at it, kill the unnecessary "if (!pid)" check in the "if (!*tid)" > block, this matches the next usage of find_pid_ns() + get_pid_task() in > this function. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > Acked-by: Yonghong Song <yonghong.song@linux.dev> Could you rebase this against bpf-next tree so this can run through our BPF CI? Right now the CI cannot pick the patch up due to merge conflict [0]. Thanks, Daniel [0] https://patchwork.kernel.org/project/netdevbpf/patch/20230822120549.GA22091@redhat.com/
On 08/25, Daniel Borkmann wrote: > > Could you rebase this against bpf-next tree so this can run through our BPF > CI? Right now the CI cannot pick the patch up due to merge conflict [0]. > > Thanks, > Daniel > > [0] https://patchwork.kernel.org/project/netdevbpf/patch/20230822120549.GA22091@redhat.com/ The merge failed because this patch depends on [PATCH] bpf: task_group_seq_get_next: cleanup the usage of next_thread() in this thread. But please forget. I've sent the new series. It would be nice if you can test at least 1-5, the last 6/6 depends on [PATCH 1/2] introduce __next_thread(), fix next_tid() vs exec() race https://lore.kernel.org/all/20230824143142.GA31222@redhat.com/ which was not merged yet. Oleg.
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c index 4d1125108014..1589ec3faded 100644 --- a/kernel/bpf/task_iter.c +++ b/kernel/bpf/task_iter.c @@ -42,9 +42,6 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm if (!*tid) { /* The first time, the iterator calls this function. */ pid = find_pid_ns(common->pid, common->ns); - if (!pid) - return NULL; - task = get_pid_task(pid, PIDTYPE_TGID); if (!task) return NULL; @@ -66,17 +63,12 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm return task; } - pid = find_pid_ns(common->pid_visiting, common->ns); - if (!pid) - return NULL; - - task = get_pid_task(pid, PIDTYPE_PID); + task = find_task_by_pid_ns(common->pid_visiting, common->ns); if (!task) return NULL; retry: next_task = next_thread(task); - put_task_struct(task); saved_tid = *tid; *tid = __task_pid_nr_ns(next_task, PIDTYPE_PID, common->ns); @@ -88,7 +80,6 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm return NULL; } - get_task_struct(next_task); common->pid_visiting = *tid; if (skip_if_dup_files && task->files == task->group_leader->files) { @@ -96,6 +87,7 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm goto retry; } + get_task_struct(next_task); return next_task; }