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

Message-ID: <657FF257-B598-4AF6-8ADB-775424A893E1@redhat.com>
Date:   Mon, 08 Jul 2019 11:02:55 +0200
From:   "Eelco Chaudron" <echaudro@...hat.com>
To:     "Magnus Karlsson" <magnus.karlsson@...il.com>
Cc:     "Daniel Borkmann" <daniel@...earbox.net>,
        "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 6 Jul 2019, at 11:57, Magnus Karlsson wrote:

> 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.

Will do this and change the example to use this new function, so it will 
work when sending single packets to it.

I’m on PTO in two days, so will do this once I’m back rather than 
try to rush it in.

>
> 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.

The problem is the xsk_ring_prod__reserve() API, it return 0 if the 
available nb’s < requested nb’s. So in order to reserve enough slots 
we have frame buffers available we need to know how many slots are 
available, hence we need the __nb_fee() function.

For the related xsk_ring_cons__peek() function we do not need this, as 
it will return the available entries requested or less.

>
> 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;
>>>
>>