netdev - Re: [RFC PATCH bpf-next 10/15] bpf: Use bpf_map_pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALOAHbBaQh38htnVg7sXrBEeCghe3RYL514xAqi_DhuHbOXMyw@mail.gmail.com>
Date:   Wed, 3 Aug 2022 21:27:59 +0800
From:   Yafang Shao <laoar.shao@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, jolsa@...nel.org,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <songmuchun@...edance.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>
Subject: Re: [RFC PATCH bpf-next 10/15] bpf: Use bpf_map_pages_alloc in ringbuf

On Wed, Aug 3, 2022 at 2:00 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Tue, Aug 2, 2022 at 6:31 AM Yafang Shao <laoar.shao@...il.com> wrote:
> >
> > On Tue, Aug 2, 2022 at 7:17 AM Andrii Nakryiko
> > <andrii.nakryiko@...il.com> wrote:
> > >
> > > On Fri, Jul 29, 2022 at 8:23 AM Yafang Shao <laoar.shao@...il.com> wrote:
> > > >
> > > > Introduce new helper bpf_map_pages_alloc() for this memory allocation.
> > > >
> > > > Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> > > > ---
> > > >  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.
> > >
> >
> > I will change it.
> >
> > >
> > > 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?
> > >
> >
> > Right, it is used in ringbuf.c only currently. I will keep it inside ringbuf.c.
> >
>
> In such case you might as well put pages = bpf_map_area_alloc(); part
> into this function and return struct page ** as a result, so that
> everything related to pages is handled as a single unit. And then
> bpf_map_pages_free() will free not just each individual page, but also
> struct page*[] array.
>

Good suggestion. I will do it.

> Also please call it something ringbuf specific, e.g.,
> bpf_ringbuf_pages_{alloc,free}()?
>

It Makes sense to me.

-- 
Regards
Yafang