Re: [PATCH bpf-next v2] libbpf: add xsk_ring_prod__nb_free() function

["Eelco Chaudron" <echaudro@redhat.com>]

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

> On Wed, Jun 26, 2019 at 10:33 AM Eelco Chaudron <echaudro@redhat.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@redhat.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
>>