* [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).