netdev - Re: [PATCH bpf-next] libbpf: add xsk_ring_prod_

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]

Date:   Mon, 24 Jun 2019 12:15:16 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Eelco Chaudron <echaudro@...hat.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin Lau <kafai@...com>, Song Liu <songliubraving@...com>,
        Yonghong Song <yhs@...com>
Subject: Re: [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function

On Mon, Jun 24, 2019 at 11:53 AM Eelco Chaudron <echaudro@...hat.com> wrote:
>
>
>
> On 21 Jun 2019, at 21:13, Andrii Nakryiko wrote:
>
> > On Fri, Jun 21, 2019 at 8:26 AM Eelco Chaudron <echaudro@...hat.com>
> > 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>
> >> ---
> >>  tools/lib/bpf/xsk.h | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
> >> index 82ea71a0f3ec..86f3d485e957 100644
> >> --- a/tools/lib/bpf/xsk.h
> >> +++ b/tools/lib/bpf/xsk.h
> >> @@ -95,6 +95,12 @@ static inline __u32 xsk_prod_nb_free(struct
> >> xsk_ring_prod *r, __u32 nb)
> >>         return r->cached_cons - r->cached_prod;
> >>  }
> >>
> >> +static inline __u32 xsk_ring_prod__free(struct xsk_ring_prod *r)
> >
> > This is a very bad name choice. __free is used for functions that free
> > memory and resources. One function below I see avail is used in the
> > name, why not xsk_ring_prog__avail?
>
> Must agree that free sound like you are freeing entries… However, I
> just kept the naming already in the API/file (see above,
> xsk_prod_nb_free()).
> Reading the code there is a difference as xx_avail() means available
> filled entries, where xx_free() means available free entries.
>
> So I could rename it to xsk_ring_prod__nb_free() maybe?

xsk_ring_prod__nb_free() is fine with me. In truth, Andrii's
suggestion is fine too since the number of available entries from the
producer point of view is the number of free entries I can put stuff
in.

Your function is expensive though since it always touches global
state. I think it would be better to expose the xsk_prod_nb_free()
function as is, but with this new name. Then users can say how many
entries they want maximum and avoid touching global state when not
needed. You would also have to change all the functions that use
xsk_prod_nb_free, so it uses you new function. What do you think?

/Magnus

> Forgot to include Magnus in the email, so copied him in, for some
> comments.
>
> >> +{
> >> +       r->cached_cons = *r->consumer + r->size;
> >> +       return r->cached_cons - r->cached_prod;
> >> +}
> >> +
> >>  static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32
> >> nb)
> >>  {
> >>         __u32 entries = r->cached_prod - r->cached_cons;
> >> --
> >> 2.20.1
> >>