Re: [PATCH bpf-next 2/3] bpf: Introduce task_vma open-coded iterator kfuncs

Stanislav Fomichev
2023-08-10T21:57:24Z

On 08/10, Dave Marchevsky wrote:
> This patch adds kfuncs bpf_iter_task_vma_{new,next,destroy} which allow
> creation and manipulation of struct bpf_iter_task_vma in open-coded
> iterator style. BPF programs can use these kfuncs directly or through
> bpf_for_each macro for natural-looking iteration of all task vmas.
>
> The implementation borrows heavily from bpf_find_vma helper's locking -
> differing only in that it holds the mmap_read lock for all iterations
> while the helper only executes its provided callback on a maximum of 1
> vma. Aside from locking, struct vma_iterator and vma_next do all the
> heavy lifting.
>
> The newly-added struct bpf_iter_task_vma has a name collision with a
> selftest for the seq_file task_vma iter's bpf skel, so the selftests/bpf/progs
> file is renamed in order to avoid the collision.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> Cc: Nathan Slingerland <slinger@meta.com>
> ---
> include/uapi/linux/bpf.h | 5 ++
> kernel/bpf/helpers.c | 3 +
> kernel/bpf/task_iter.c | 56 +++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 5 ++
> tools/lib/bpf/bpf_helpers.h | 8 +++
> .../selftests/bpf/prog_tests/bpf_iter.c | 26 ++++-----
> ...f_iter_task_vma.c => bpf_iter_task_vmas.c} | 0
> 7 files changed, 90 insertions(+), 13 deletions(-)
> rename tools/testing/selftests/bpf/progs/{bpf_iter_task_vma.c => bpf_iter_task_vmas.c} (100%)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index d21deb46f49f..c4a65968f9f5 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7291,4 +7291,9 @@ struct bpf_iter_num {
> __u64 __opaque[1];
> } __attribute__((aligned(8)));
>
> +struct bpf_iter_task_vma {

[..]

> + __u64 __opaque[9]; /* See bpf_iter_num comment above */
> + char __opaque_c[3];

Everything in the series makes sense, but this part is a big confusing
when reading without too much context. If you're gonna do a respin, maybe:

- __opaque_c[8*9+3] (or whatever the size is)? any reason for separate
__u64 + char?
- maybe worth adding something like /* Opaque representation of
bpf_iter_task_vma_kern; see bpf_iter_num comment above */.
that bpf_iter_task_vma<>bpf_iter_task_vma_kern wasn't super apparent
until I got to the BUG_ON part