netdev - Re: [PATCH bpf-next v2] 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: <B0DB9024-43D8-4DBA-A588-15154B82D413@redhat.com>
Date:   Wed, 03 Jul 2019 11:10:30 +0200
From:   "Eelco Chaudron" <echaudro@...hat.com>
To:     "Magnus Karlsson" <magnus.karlsson@...il.com>
Cc:     "Network Development" <netdev@...r.kernel.org>,
        "Alexei Starovoitov" <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        "Martin KaFai Lau" <kafai@...com>,
        "Song Liu" <songliubraving@...com>, "Yonghong Song" <yhs@...com>,
        "Andrii Nakryiko" <andrii.nakryiko@...il.com>
Subject: Re: [PATCH bpf-next v2] libbpf: add xsk_ring_prod__nb_free() function



On 28 Jun 2019, at 12:14, Magnus Karlsson wrote:

> On Wed, Jun 26, 2019 at 10:33 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>
>> ---
>>
>> 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 | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
>> index 82ea71a0f3ec..6acb81102346 100644
>> --- a/tools/lib/bpf/xsk.h
>> +++ b/tools/lib/bpf/xsk.h
>> @@ -76,11 +76,11 @@ 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;
>>
>> -       if (free_entries >= nb)
>> +       if (free_entries >= nb && nb != 0)
>>                 return free_entries;
>
> Thanks Eelco for the patch. Is the test nb != 0 introduced here so
> that the function will continue with the refresh from the global state
> when nb is set to 0? If so, could a user not instead just set the nb
> parameter to the size of the ring? This would always trigger a
> refresh, except when the number of free entries is equal to the size
> of the ring, but then we do not need the refresh anyway. This would
> eliminate the nb != 0 test that you introduced from the fast path.

Will remove this change from the fast path, and your suggestion can be 
used if circumvention of the cache is needed. Will sent out a v3 soon...

>>         /* Refresh the local tail pointer.
>> @@ -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;
>> --
>> 2.20.1
>>