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

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

[Eelco Chaudron]

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

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

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

[Eelco Chaudron]

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

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

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

[Daniel Borkmann]

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

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

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

[Magnus Karlsson]

On Fri, Jul 5, 2019 at 4:35 PM Daniel Borkmann <daniel@iogearbox.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@redhat.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;
> >
>

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

[Eelco Chaudron]

On 6 Jul 2019, at 11:57, Magnus Karlsson wrote:

> On Fri, Jul 5, 2019 at 4:35 PM Daniel Borkmann <daniel@iogearbox.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@redhat.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;
>>>
>>