netdev - Re: [PATCH bpf-next v3] libbpf: add xsk_ring_prod__nb

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ8uoz0LjXMaVgnf7_UkfRwN2Dx11m1Th5FXyf1vgGWDd5Tswg@mail.gmail.com>
Date:   Sat, 6 Jul 2019 11:57:32 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Eelco Chaudron <echaudro@...hat.com>,
        Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: [PATCH bpf-next v3] libbpf: add xsk_ring_prod__nb_free() function

On Fri, Jul 5, 2019 at 4:35 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 07/03/2019 02:52 PM, Eelco Chaudron wrote:
> > When an AF_XDP application received X packets, it does not mean X
> > frames can be stuffed into the producer ring. To make it easier for
> > AF_XDP applications this API allows them to check how many frames can
> > be added into the ring.
> >
> > Signed-off-by: Eelco Chaudron <echaudro@...hat.com>
>
> The commit log as it is along with the code is a bit too confusing for
> readers. After all you only do a rename below. It would need to additionally
> state that the rename is as per libbpf convention (xyz__ prefix) in order to
> denote that this API is exposed to be used by applications.
>
> Given you are doing this for xsk_prod_nb_free(), should we do the same for
> xsk_cons_nb_avail() as well? Extending XDP sample app would be reasonable
> addition as well in this context.

Sorry for the late reply Eelco. My e-mail filter is apparently not set
up correctly since it does not catch mails where I am on the CC line.
Will fix.

At the same time you are rewording the commit log according to
Daniel's suggestion, could you please also add a line or two
explaining how to use the nb parameter? If you set it to the size of
the ring, you will get the exact amount of slots available, at the
cost of performance (you touch shared state for sure). nb is there to
limit the touching of shared state. The same kind of comment in the
header file would be great too.

Have you found any use of the  xsk_cons_nb_avail() function from your
sample application? If so, let us add it to the public API.

Thanks: Magnus

> > ---
> >
> > v2 -> v3
> >  - Removed cache by pass option
> >
> > v1 -> v2
> >  - Renamed xsk_ring_prod__free() to xsk_ring_prod__nb_free()
> >  - Add caching so it will only touch global state when needed
> >
> >  tools/lib/bpf/xsk.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
> > index 82ea71a0f3ec..3411556e04d9 100644
> > --- a/tools/lib/bpf/xsk.h
> > +++ b/tools/lib/bpf/xsk.h
> > @@ -76,7 +76,7 @@ xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
> >       return &descs[idx & rx->mask];
> >  }
> >
> > -static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
> > +static inline __u32 xsk_prod__nb_free(struct xsk_ring_prod *r, __u32 nb)
> >  {
> >       __u32 free_entries = r->cached_cons - r->cached_prod;
> >
> > @@ -110,7 +110,7 @@ static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
> >  static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod,
> >                                           size_t nb, __u32 *idx)
> >  {
> > -     if (xsk_prod_nb_free(prod, nb) < nb)
> > +     if (xsk_prod__nb_free(prod, nb) < nb)
> >               return 0;
> >
> >       *idx = prod->cached_prod;
> >
>