* [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function
@ 2019-06-21 15:25 Eelco Chaudron
2019-06-21 19:13 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Eelco Chaudron @ 2019-06-21 15:25 UTC (permalink / raw)
To: netdev; +Cc: ast, daniel, kafai, songliubraving, yhs
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>
---
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)
+{
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function
2019-06-21 15:25 [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function Eelco Chaudron
@ 2019-06-21 19:13 ` Andrii Nakryiko
2019-06-24 9:37 ` Eelco Chaudron
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2019-06-21 19:13 UTC (permalink / raw)
To: Eelco Chaudron
Cc: Networking, Alexei Starovoitov, Daniel Borkmann, Martin Lau,
Song Liu, Yonghong Song
On Fri, Jun 21, 2019 at 8:26 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>
> ---
> 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?
> +{
> + 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function
2019-06-21 19:13 ` Andrii Nakryiko
@ 2019-06-24 9:37 ` Eelco Chaudron
2019-06-24 10:15 ` Magnus Karlsson
2019-06-24 16:42 ` Andrii Nakryiko
0 siblings, 2 replies; 6+ messages in thread
From: Eelco Chaudron @ 2019-06-24 9:37 UTC (permalink / raw)
To: Andrii Nakryiko, Karlsson, Magnus
Cc: Networking, Alexei Starovoitov, Daniel Borkmann, Martin Lau,
Song Liu, Yonghong Song
On 21 Jun 2019, at 21:13, Andrii Nakryiko wrote:
> On Fri, Jun 21, 2019 at 8:26 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>
>> ---
>> 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?
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function
2019-06-24 9:37 ` Eelco Chaudron
@ 2019-06-24 10:15 ` Magnus Karlsson
2019-06-24 16:42 ` Andrii Nakryiko
1 sibling, 0 replies; 6+ messages in thread
From: Magnus Karlsson @ 2019-06-24 10:15 UTC (permalink / raw)
To: Eelco Chaudron
Cc: Andrii Nakryiko, Karlsson, Magnus, Networking,
Alexei Starovoitov, Daniel Borkmann, Martin Lau, Song Liu,
Yonghong Song
On Mon, Jun 24, 2019 at 11:53 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
>
>
> On 21 Jun 2019, at 21:13, Andrii Nakryiko wrote:
>
> > On Fri, Jun 21, 2019 at 8:26 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>
> >> ---
> >> 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
> >>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function
2019-06-24 9:37 ` Eelco Chaudron
2019-06-24 10:15 ` Magnus Karlsson
@ 2019-06-24 16:42 ` Andrii Nakryiko
2019-06-25 7:58 ` Eelco Chaudron
1 sibling, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2019-06-24 16:42 UTC (permalink / raw)
To: Eelco Chaudron
Cc: Karlsson, Magnus, Networking, Alexei Starovoitov,
Daniel Borkmann, Martin Lau, Song Liu, Yonghong Song
On Mon, Jun 24, 2019 at 2:37 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
>
>
> On 21 Jun 2019, at 21:13, Andrii Nakryiko wrote:
>
> > On Fri, Jun 21, 2019 at 8:26 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>
> >> ---
> >> 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?
I'm fine with __nb_free, yes. Thanks!
>
> 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
> >>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function
2019-06-24 16:42 ` Andrii Nakryiko
@ 2019-06-25 7:58 ` Eelco Chaudron
0 siblings, 0 replies; 6+ messages in thread
From: Eelco Chaudron @ 2019-06-25 7:58 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Karlsson, Magnus, Networking, Alexei Starovoitov,
Daniel Borkmann, Martin Lau, Song Liu, Yonghong Song
On 24 Jun 2019, at 18:42, Andrii Nakryiko wrote:
> On Mon, Jun 24, 2019 at 2:37 AM Eelco Chaudron <echaudro@redhat.com>
> wrote:
>>
>>
>>
>> On 21 Jun 2019, at 21:13, Andrii Nakryiko wrote:
>>
>>> On Fri, Jun 21, 2019 at 8:26 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>
>>>> ---
>>>> 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?
>
> I'm fine with __nb_free, yes. Thanks!
Ok, will rework the patch and use xsk_ring_prod__nb_free(). Will also
take Magnus suggestion into account, and create a cached version (and
use it internally).
>>
>> 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
>>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-25 7:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-21 15:25 [PATCH bpf-next] libbpf: add xsk_ring_prod__free() function Eelco Chaudron
2019-06-21 19:13 ` Andrii Nakryiko
2019-06-24 9:37 ` Eelco Chaudron
2019-06-24 10:15 ` Magnus Karlsson
2019-06-24 16:42 ` Andrii Nakryiko
2019-06-25 7:58 ` Eelco Chaudron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).