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

[Stanislav Fomichev <sdf@google.com>]

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