On Fri, Jul 29, 2022 at 8:23 AM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> Introduce new helper bpf_map_pages_alloc() for this memory allocation.
>
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> ---
> include/linux/bpf.h | 4 ++++
> kernel/bpf/ringbuf.c | 27 +++++++++------------------
> kernel/bpf/syscall.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 54 insertions(+), 18 deletions(-)
>
[...]
> /* Each data page is mapped twice to allow "virtual"
> * continuous read of samples wrapping around the end of ring
> @@ -95,16 +95,10 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(struct bpf_map *map,
> if (!pages)
> return NULL;
>
> - for (i = 0; i < nr_pages; i++) {
> - page = alloc_pages_node(numa_node, flags, 0);
> - if (!page) {
> - nr_pages = i;
> - goto err_free_pages;
> - }
> - pages[i] = page;
> - if (i >= nr_meta_pages)
> - pages[nr_data_pages + i] = page;
> - }
> + ptr = bpf_map_pages_alloc(map, pages, nr_meta_pages, nr_data_pages,
> + numa_node, flags, 0);
> + if (!ptr)
bpf_map_pages_alloc() has some weird and confusing interface. It fills
out pages (second argument) and also returns pages as void *. Why not
just return int error (0 or -ENOMEM)? You are discarding this ptr
anyways.
But also thinking some more, bpf_map_pages_alloc() is very ringbuf
specific (which other map will have exactly the same meaning for
nr_meta_pages and nr_data_pages, where we also allocate 2 *
nr_data_pages, etc).
I don't think it makes sense to expose it as a generic internal API.
Why not keep all that inside kernel/bpf/ringbuf.c instead?
> + goto err_free_pages;
>
> rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> VM_MAP | VM_USERMAP, PAGE_KERNEL);
[...]
