* [PATCH][V2] libbpf: add support for canceling cached_cons advance @ 2020-11-24 7:21 Li RongQing 2020-11-24 8:12 ` Magnus Karlsson 0 siblings, 1 reply; 9+ messages in thread From: Li RongQing @ 2020-11-24 7:21 UTC (permalink / raw) To: netdev, bpf, magnus.karlsson Add a new function for returning descriptors the user received after an xsk_ring_cons__peek call. After the application has gotten a number of descriptors from a ring, it might not be able to or want to process them all for various reasons. Therefore, it would be useful to have an interface for returning or cancelling a number of them so that they are returned to the ring. This patch adds a new function called xsk_ring_cons__cancel that performs this operation on nb descriptors counted from the end of the batch of descriptors that was received through the peek call. Signed-off-by: Li RongQing <lirongqing@baidu.com> [ Magnus Karlsson: rewrote changelog ] Cc: Magnus Karlsson <magnus.karlsson@intel.com> --- diff with v1: fix the building, and rewrote changelog 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 1069c46364ff..1719a327e5f9 100644 --- a/tools/lib/bpf/xsk.h +++ b/tools/lib/bpf/xsk.h @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, return entries; } +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, + size_t nb) +{ + cons->cached_cons -= nb; +} + static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) { /* Make sure data has been read before indicating we are done -- 2.17.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-24 7:21 [PATCH][V2] libbpf: add support for canceling cached_cons advance Li RongQing @ 2020-11-24 8:12 ` Magnus Karlsson 2020-11-24 21:58 ` Daniel Borkmann 0 siblings, 1 reply; 9+ messages in thread From: Magnus Karlsson @ 2020-11-24 8:12 UTC (permalink / raw) To: Li RongQing; +Cc: Network Development, bpf, Karlsson, Magnus On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: > > Add a new function for returning descriptors the user received > after an xsk_ring_cons__peek call. After the application has > gotten a number of descriptors from a ring, it might not be able > to or want to process them all for various reasons. Therefore, > it would be useful to have an interface for returning or > cancelling a number of them so that they are returned to the ring. > > This patch adds a new function called xsk_ring_cons__cancel that > performs this operation on nb descriptors counted from the end of > the batch of descriptors that was received through the peek call. > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > [ Magnus Karlsson: rewrote changelog ] > Cc: Magnus Karlsson <magnus.karlsson@intel.com> > --- > diff with v1: fix the building, and rewrote changelog > > 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 1069c46364ff..1719a327e5f9 100644 > --- a/tools/lib/bpf/xsk.h > +++ b/tools/lib/bpf/xsk.h > @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, > return entries; > } > > +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, > + size_t nb) > +{ > + cons->cached_cons -= nb; > +} > + > static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) > { > /* Make sure data has been read before indicating we are done > -- > 2.17.3 Thank you RongQing. Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-24 8:12 ` Magnus Karlsson @ 2020-11-24 21:58 ` Daniel Borkmann 2020-11-25 8:30 ` Magnus Karlsson 0 siblings, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2020-11-24 21:58 UTC (permalink / raw) To: Magnus Karlsson, Li RongQing; +Cc: Network Development, bpf, Karlsson, Magnus On 11/24/20 9:12 AM, Magnus Karlsson wrote: > On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: >> >> Add a new function for returning descriptors the user received >> after an xsk_ring_cons__peek call. After the application has >> gotten a number of descriptors from a ring, it might not be able >> to or want to process them all for various reasons. Therefore, >> it would be useful to have an interface for returning or >> cancelling a number of them so that they are returned to the ring. >> >> This patch adds a new function called xsk_ring_cons__cancel that >> performs this operation on nb descriptors counted from the end of >> the batch of descriptors that was received through the peek call. >> >> Signed-off-by: Li RongQing <lirongqing@baidu.com> >> [ Magnus Karlsson: rewrote changelog ] >> Cc: Magnus Karlsson <magnus.karlsson@intel.com> >> --- >> diff with v1: fix the building, and rewrote changelog >> >> 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 1069c46364ff..1719a327e5f9 100644 >> --- a/tools/lib/bpf/xsk.h >> +++ b/tools/lib/bpf/xsk.h >> @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, >> return entries; >> } >> >> +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, >> + size_t nb) >> +{ >> + cons->cached_cons -= nb; >> +} >> + >> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) >> { >> /* Make sure data has been read before indicating we are done >> -- >> 2.17.3 > > Thank you RongQing. > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> @Magnus: shouldn't the xsk_ring_cons__cancel() nb type be '__u32 nb' instead? Thanks, Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-24 21:58 ` Daniel Borkmann @ 2020-11-25 8:30 ` Magnus Karlsson 2020-11-25 9:02 ` Daniel Borkmann 0 siblings, 1 reply; 9+ messages in thread From: Magnus Karlsson @ 2020-11-25 8:30 UTC (permalink / raw) To: Daniel Borkmann; +Cc: Li RongQing, Network Development, bpf, Karlsson, Magnus On Tue, Nov 24, 2020 at 10:58 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 11/24/20 9:12 AM, Magnus Karlsson wrote: > > On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: > >> > >> Add a new function for returning descriptors the user received > >> after an xsk_ring_cons__peek call. After the application has > >> gotten a number of descriptors from a ring, it might not be able > >> to or want to process them all for various reasons. Therefore, > >> it would be useful to have an interface for returning or > >> cancelling a number of them so that they are returned to the ring. > >> > >> This patch adds a new function called xsk_ring_cons__cancel that > >> performs this operation on nb descriptors counted from the end of > >> the batch of descriptors that was received through the peek call. > >> > >> Signed-off-by: Li RongQing <lirongqing@baidu.com> > >> [ Magnus Karlsson: rewrote changelog ] > >> Cc: Magnus Karlsson <magnus.karlsson@intel.com> > >> --- > >> diff with v1: fix the building, and rewrote changelog > >> > >> 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 1069c46364ff..1719a327e5f9 100644 > >> --- a/tools/lib/bpf/xsk.h > >> +++ b/tools/lib/bpf/xsk.h > >> @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, > >> return entries; > >> } > >> > >> +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, > >> + size_t nb) > >> +{ > >> + cons->cached_cons -= nb; > >> +} > >> + > >> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) > >> { > >> /* Make sure data has been read before indicating we are done > >> -- > >> 2.17.3 > > > > Thank you RongQing. > > > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > > @Magnus: shouldn't the xsk_ring_cons__cancel() nb type be '__u32 nb' instead? All the other interfaces have size_t as the type for "nb". It is kind of weird as a __u32 would have made more sense, but cannot actually remember why I chose a size_t two years ago. But for consistency with the other interfaces, let us keep it a size_t for now. I will do some research around the reason. > Thanks, > Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-25 8:30 ` Magnus Karlsson @ 2020-11-25 9:02 ` Daniel Borkmann 2020-11-25 9:13 ` Magnus Karlsson 0 siblings, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2020-11-25 9:02 UTC (permalink / raw) To: Magnus Karlsson; +Cc: Li RongQing, Network Development, bpf, Karlsson, Magnus On 11/25/20 9:30 AM, Magnus Karlsson wrote: > On Tue, Nov 24, 2020 at 10:58 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 11/24/20 9:12 AM, Magnus Karlsson wrote: >>> On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: >>>> >>>> Add a new function for returning descriptors the user received >>>> after an xsk_ring_cons__peek call. After the application has >>>> gotten a number of descriptors from a ring, it might not be able >>>> to or want to process them all for various reasons. Therefore, >>>> it would be useful to have an interface for returning or >>>> cancelling a number of them so that they are returned to the ring. >>>> >>>> This patch adds a new function called xsk_ring_cons__cancel that >>>> performs this operation on nb descriptors counted from the end of >>>> the batch of descriptors that was received through the peek call. >>>> >>>> Signed-off-by: Li RongQing <lirongqing@baidu.com> >>>> [ Magnus Karlsson: rewrote changelog ] >>>> Cc: Magnus Karlsson <magnus.karlsson@intel.com> >>>> --- >>>> diff with v1: fix the building, and rewrote changelog >>>> >>>> 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 1069c46364ff..1719a327e5f9 100644 >>>> --- a/tools/lib/bpf/xsk.h >>>> +++ b/tools/lib/bpf/xsk.h >>>> @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, >>>> return entries; >>>> } >>>> >>>> +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, >>>> + size_t nb) >>>> +{ >>>> + cons->cached_cons -= nb; >>>> +} >>>> + >>>> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) >>>> { >>>> /* Make sure data has been read before indicating we are done >>>> -- >>>> 2.17.3 >>> >>> Thank you RongQing. >>> >>> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> >> >> @Magnus: shouldn't the xsk_ring_cons__cancel() nb type be '__u32 nb' instead? > > All the other interfaces have size_t as the type for "nb". It is kind > of weird as a __u32 would have made more sense, but cannot actually > remember why I chose a size_t two years ago. But for consistency with > the other interfaces, let us keep it a size_t for now. I will do some > research around the reason. It's actually a bit of a mix currently which is what got me confused: static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb) 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) static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb) static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, size_t nb, __u32 *idx) static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) (I can take it in as-is, but would be nice to clean it up a bit to avoid confusion.) Thanks, Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-25 9:02 ` Daniel Borkmann @ 2020-11-25 9:13 ` Magnus Karlsson 2020-11-25 10:06 ` Daniel Borkmann 0 siblings, 1 reply; 9+ messages in thread From: Magnus Karlsson @ 2020-11-25 9:13 UTC (permalink / raw) To: Daniel Borkmann; +Cc: Li RongQing, Network Development, bpf, Karlsson, Magnus On Wed, Nov 25, 2020 at 10:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 11/25/20 9:30 AM, Magnus Karlsson wrote: > > On Tue, Nov 24, 2020 at 10:58 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> On 11/24/20 9:12 AM, Magnus Karlsson wrote: > >>> On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: > >>>> > >>>> Add a new function for returning descriptors the user received > >>>> after an xsk_ring_cons__peek call. After the application has > >>>> gotten a number of descriptors from a ring, it might not be able > >>>> to or want to process them all for various reasons. Therefore, > >>>> it would be useful to have an interface for returning or > >>>> cancelling a number of them so that they are returned to the ring. > >>>> > >>>> This patch adds a new function called xsk_ring_cons__cancel that > >>>> performs this operation on nb descriptors counted from the end of > >>>> the batch of descriptors that was received through the peek call. > >>>> > >>>> Signed-off-by: Li RongQing <lirongqing@baidu.com> > >>>> [ Magnus Karlsson: rewrote changelog ] > >>>> Cc: Magnus Karlsson <magnus.karlsson@intel.com> > >>>> --- > >>>> diff with v1: fix the building, and rewrote changelog > >>>> > >>>> 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 1069c46364ff..1719a327e5f9 100644 > >>>> --- a/tools/lib/bpf/xsk.h > >>>> +++ b/tools/lib/bpf/xsk.h > >>>> @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, > >>>> return entries; > >>>> } > >>>> > >>>> +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, > >>>> + size_t nb) > >>>> +{ > >>>> + cons->cached_cons -= nb; > >>>> +} > >>>> + > >>>> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) > >>>> { > >>>> /* Make sure data has been read before indicating we are done > >>>> -- > >>>> 2.17.3 > >>> > >>> Thank you RongQing. > >>> > >>> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > >> > >> @Magnus: shouldn't the xsk_ring_cons__cancel() nb type be '__u32 nb' instead? > > > > All the other interfaces have size_t as the type for "nb". It is kind > > of weird as a __u32 would have made more sense, but cannot actually > > remember why I chose a size_t two years ago. But for consistency with > > the other interfaces, let us keep it a size_t for now. I will do some > > research around the reason. > > It's actually a bit of a mix currently which is what got me confused: > > static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb) > 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) > static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb) > static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, size_t nb, __u32 *idx) > static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) > > (I can take it in as-is, but would be nice to clean it up a bit to avoid confusion.) Hmm, that is confusing indeed. Well, the best choice would be __u32 everywhere since the ring pointers themselves are __u32. But I am somewhat afraid of changing an API. Can we guarantee that a change from size_t to __u32 will not break some user's compilation? Another option would be to clean this up next year when we will very likely produce a 1.0 version of this API and at that point we can change some things. What do you think would be the best approach? > Thanks, > Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-25 9:13 ` Magnus Karlsson @ 2020-11-25 10:06 ` Daniel Borkmann 2020-11-25 10:09 ` Magnus Karlsson 0 siblings, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2020-11-25 10:06 UTC (permalink / raw) To: Magnus Karlsson; +Cc: Li RongQing, Network Development, bpf, Karlsson, Magnus On 11/25/20 10:13 AM, Magnus Karlsson wrote: > On Wed, Nov 25, 2020 at 10:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 11/25/20 9:30 AM, Magnus Karlsson wrote: >>> On Tue, Nov 24, 2020 at 10:58 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >>>> On 11/24/20 9:12 AM, Magnus Karlsson wrote: >>>>> On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: >>>>>> >>>>>> Add a new function for returning descriptors the user received >>>>>> after an xsk_ring_cons__peek call. After the application has >>>>>> gotten a number of descriptors from a ring, it might not be able >>>>>> to or want to process them all for various reasons. Therefore, >>>>>> it would be useful to have an interface for returning or >>>>>> cancelling a number of them so that they are returned to the ring. >>>>>> >>>>>> This patch adds a new function called xsk_ring_cons__cancel that >>>>>> performs this operation on nb descriptors counted from the end of >>>>>> the batch of descriptors that was received through the peek call. >>>>>> >>>>>> Signed-off-by: Li RongQing <lirongqing@baidu.com> >>>>>> [ Magnus Karlsson: rewrote changelog ] >>>>>> Cc: Magnus Karlsson <magnus.karlsson@intel.com> >>>>>> --- >>>>>> diff with v1: fix the building, and rewrote changelog >>>>>> >>>>>> 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 1069c46364ff..1719a327e5f9 100644 >>>>>> --- a/tools/lib/bpf/xsk.h >>>>>> +++ b/tools/lib/bpf/xsk.h >>>>>> @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, >>>>>> return entries; >>>>>> } >>>>>> >>>>>> +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, >>>>>> + size_t nb) >>>>>> +{ >>>>>> + cons->cached_cons -= nb; >>>>>> +} >>>>>> + >>>>>> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) >>>>>> { >>>>>> /* Make sure data has been read before indicating we are done >>>>>> -- >>>>>> 2.17.3 >>>>> >>>>> Thank you RongQing. >>>>> >>>>> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> >>>> >>>> @Magnus: shouldn't the xsk_ring_cons__cancel() nb type be '__u32 nb' instead? >>> >>> All the other interfaces have size_t as the type for "nb". It is kind >>> of weird as a __u32 would have made more sense, but cannot actually >>> remember why I chose a size_t two years ago. But for consistency with >>> the other interfaces, let us keep it a size_t for now. I will do some >>> research around the reason. >> >> It's actually a bit of a mix currently which is what got me confused: >> >> static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb) >> 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) >> static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb) >> static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, size_t nb, __u32 *idx) >> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) >> >> (I can take it in as-is, but would be nice to clean it up a bit to avoid confusion.) > > Hmm, that is confusing indeed. Well, the best choice would be __u32 > everywhere since the ring pointers themselves are __u32. But I am > somewhat afraid of changing an API. Can we guarantee that a change > from size_t to __u32 will not break some user's compilation? Another > option would be to clean this up next year when we will very likely > produce a 1.0 version of this API and at that point we can change some > things. What do you think would be the best approach? Given they're all inlines, imho, risk should be fairly low to switch all to __u32. I would probably go and verify first with DPDK as main user of the lib and/or write some test cases to see if compiler spills any new warnings and the like, but if not the case then we should do it for bpf-next so this has plenty of exposure in the meantime. Any nb large than u32 max is a bug in any case. Thanks, Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-25 10:06 ` Daniel Borkmann @ 2020-11-25 10:09 ` Magnus Karlsson 2020-11-25 12:22 ` Daniel Borkmann 0 siblings, 1 reply; 9+ messages in thread From: Magnus Karlsson @ 2020-11-25 10:09 UTC (permalink / raw) To: Daniel Borkmann; +Cc: Li RongQing, Network Development, bpf, Karlsson, Magnus On Wed, Nov 25, 2020 at 11:07 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 11/25/20 10:13 AM, Magnus Karlsson wrote: > > On Wed, Nov 25, 2020 at 10:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> On 11/25/20 9:30 AM, Magnus Karlsson wrote: > >>> On Tue, Nov 24, 2020 at 10:58 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > >>>> On 11/24/20 9:12 AM, Magnus Karlsson wrote: > >>>>> On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: > >>>>>> > >>>>>> Add a new function for returning descriptors the user received > >>>>>> after an xsk_ring_cons__peek call. After the application has > >>>>>> gotten a number of descriptors from a ring, it might not be able > >>>>>> to or want to process them all for various reasons. Therefore, > >>>>>> it would be useful to have an interface for returning or > >>>>>> cancelling a number of them so that they are returned to the ring. > >>>>>> > >>>>>> This patch adds a new function called xsk_ring_cons__cancel that > >>>>>> performs this operation on nb descriptors counted from the end of > >>>>>> the batch of descriptors that was received through the peek call. > >>>>>> > >>>>>> Signed-off-by: Li RongQing <lirongqing@baidu.com> > >>>>>> [ Magnus Karlsson: rewrote changelog ] > >>>>>> Cc: Magnus Karlsson <magnus.karlsson@intel.com> > >>>>>> --- > >>>>>> diff with v1: fix the building, and rewrote changelog > >>>>>> > >>>>>> 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 1069c46364ff..1719a327e5f9 100644 > >>>>>> --- a/tools/lib/bpf/xsk.h > >>>>>> +++ b/tools/lib/bpf/xsk.h > >>>>>> @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, > >>>>>> return entries; > >>>>>> } > >>>>>> > >>>>>> +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, > >>>>>> + size_t nb) > >>>>>> +{ > >>>>>> + cons->cached_cons -= nb; > >>>>>> +} > >>>>>> + > >>>>>> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) > >>>>>> { > >>>>>> /* Make sure data has been read before indicating we are done > >>>>>> -- > >>>>>> 2.17.3 > >>>>> > >>>>> Thank you RongQing. > >>>>> > >>>>> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > >>>> > >>>> @Magnus: shouldn't the xsk_ring_cons__cancel() nb type be '__u32 nb' instead? > >>> > >>> All the other interfaces have size_t as the type for "nb". It is kind > >>> of weird as a __u32 would have made more sense, but cannot actually > >>> remember why I chose a size_t two years ago. But for consistency with > >>> the other interfaces, let us keep it a size_t for now. I will do some > >>> research around the reason. > >> > >> It's actually a bit of a mix currently which is what got me confused: > >> > >> static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb) > >> 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) > >> static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb) > >> static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, size_t nb, __u32 *idx) > >> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) > >> > >> (I can take it in as-is, but would be nice to clean it up a bit to avoid confusion.) > > > > Hmm, that is confusing indeed. Well, the best choice would be __u32 > > everywhere since the ring pointers themselves are __u32. But I am > > somewhat afraid of changing an API. Can we guarantee that a change > > from size_t to __u32 will not break some user's compilation? Another > > option would be to clean this up next year when we will very likely > > produce a 1.0 version of this API and at that point we can change some > > things. What do you think would be the best approach? > > Given they're all inlines, imho, risk should be fairly low to switch all to __u32. > I would probably go and verify first with DPDK as main user of the lib and/or write > some test cases to see if compiler spills any new warnings and the like, but if not > the case then we should do it for bpf-next so this has plenty of exposure in the > meantime. Any nb large than u32 max is a bug in any case. Sounds good. Will do and get back to you. > Thanks, > Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][V2] libbpf: add support for canceling cached_cons advance 2020-11-25 10:09 ` Magnus Karlsson @ 2020-11-25 12:22 ` Daniel Borkmann 0 siblings, 0 replies; 9+ messages in thread From: Daniel Borkmann @ 2020-11-25 12:22 UTC (permalink / raw) To: Magnus Karlsson; +Cc: Li RongQing, Network Development, bpf, Karlsson, Magnus On 11/25/20 11:09 AM, Magnus Karlsson wrote: > On Wed, Nov 25, 2020 at 11:07 AM Daniel Borkmann <daniel@iogearbox.net> wrote: >> >> On 11/25/20 10:13 AM, Magnus Karlsson wrote: >>> On Wed, Nov 25, 2020 at 10:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote: >>>> On 11/25/20 9:30 AM, Magnus Karlsson wrote: >>>>> On Tue, Nov 24, 2020 at 10:58 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >>>>>> On 11/24/20 9:12 AM, Magnus Karlsson wrote: >>>>>>> On Tue, Nov 24, 2020 at 8:33 AM Li RongQing <lirongqing@baidu.com> wrote: >>>>>>>> >>>>>>>> Add a new function for returning descriptors the user received >>>>>>>> after an xsk_ring_cons__peek call. After the application has >>>>>>>> gotten a number of descriptors from a ring, it might not be able >>>>>>>> to or want to process them all for various reasons. Therefore, >>>>>>>> it would be useful to have an interface for returning or >>>>>>>> cancelling a number of them so that they are returned to the ring. >>>>>>>> >>>>>>>> This patch adds a new function called xsk_ring_cons__cancel that >>>>>>>> performs this operation on nb descriptors counted from the end of >>>>>>>> the batch of descriptors that was received through the peek call. >>>>>>>> >>>>>>>> Signed-off-by: Li RongQing <lirongqing@baidu.com> >>>>>>>> [ Magnus Karlsson: rewrote changelog ] >>>>>>>> Cc: Magnus Karlsson <magnus.karlsson@intel.com> >>>>>>>> --- >>>>>>>> diff with v1: fix the building, and rewrote changelog >>>>>>>> >>>>>>>> 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 1069c46364ff..1719a327e5f9 100644 >>>>>>>> --- a/tools/lib/bpf/xsk.h >>>>>>>> +++ b/tools/lib/bpf/xsk.h >>>>>>>> @@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, >>>>>>>> return entries; >>>>>>>> } >>>>>>>> >>>>>>>> +static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons, >>>>>>>> + size_t nb) >>>>>>>> +{ >>>>>>>> + cons->cached_cons -= nb; >>>>>>>> +} >>>>>>>> + >>>>>>>> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) >>>>>>>> { >>>>>>>> /* Make sure data has been read before indicating we are done >>>>>>>> -- >>>>>>>> 2.17.3 >>>>>>> >>>>>>> Thank you RongQing. >>>>>>> >>>>>>> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> >>>>>> >>>>>> @Magnus: shouldn't the xsk_ring_cons__cancel() nb type be '__u32 nb' instead? >>>>> >>>>> All the other interfaces have size_t as the type for "nb". It is kind >>>>> of weird as a __u32 would have made more sense, but cannot actually >>>>> remember why I chose a size_t two years ago. But for consistency with >>>>> the other interfaces, let us keep it a size_t for now. I will do some >>>>> research around the reason. >>>> >>>> It's actually a bit of a mix currently which is what got me confused: >>>> >>>> static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb) >>>> 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) >>>> static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t nb) >>>> static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons, size_t nb, __u32 *idx) >>>> static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb) >>>> >>>> (I can take it in as-is, but would be nice to clean it up a bit to avoid confusion.) >>> >>> Hmm, that is confusing indeed. Well, the best choice would be __u32 >>> everywhere since the ring pointers themselves are __u32. But I am >>> somewhat afraid of changing an API. Can we guarantee that a change >>> from size_t to __u32 will not break some user's compilation? Another >>> option would be to clean this up next year when we will very likely >>> produce a 1.0 version of this API and at that point we can change some >>> things. What do you think would be the best approach? >> >> Given they're all inlines, imho, risk should be fairly low to switch all to __u32. >> I would probably go and verify first with DPDK as main user of the lib and/or write >> some test cases to see if compiler spills any new warnings and the like, but if not >> the case then we should do it for bpf-next so this has plenty of exposure in the >> meantime. Any nb large than u32 max is a bug in any case. > > Sounds good. Will do and get back to you. Great, thanks, I took in the current patch to bpf-next in that case and the rest can be followed-up as discussed. Thanks, Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-25 12:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-24 7:21 [PATCH][V2] libbpf: add support for canceling cached_cons advance Li RongQing 2020-11-24 8:12 ` Magnus Karlsson 2020-11-24 21:58 ` Daniel Borkmann 2020-11-25 8:30 ` Magnus Karlsson 2020-11-25 9:02 ` Daniel Borkmann 2020-11-25 9:13 ` Magnus Karlsson 2020-11-25 10:06 ` Daniel Borkmann 2020-11-25 10:09 ` Magnus Karlsson 2020-11-25 12:22 ` Daniel Borkmann
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).