[PATCH] libbpf: add support for canceling cached_cons advance

[PATCH] libbpf: add support for canceling cached_cons advance

[Li RongQing]

It is possible to fail receiving packets after calling
xsk_ring_cons__peek, at this condition, cached_cons has
been advanced, should be cancelled.

Signed-off-by: Li RongQing <lirongqing@baidu.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 1069c46364ff..4128215c246b 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)
+{
+	rx->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

Re: [PATCH] libbpf: add support for canceling cached_cons advance

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

Hi Li,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master ipvs/master v5.10-rc4 next-20201120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Li-RongQing/libbpf-add-support-for-canceling-cached_cons-advance/20201122-212415
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a006-20201122 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/394b6e92389887aaf0ef8a2a70ef295b045c748b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Li-RongQing/libbpf-add-support-for-canceling-cached_cons-advance/20201122-212415
        git checkout 394b6e92389887aaf0ef8a2a70ef295b045c748b
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from xsk.c:35:
   xsk.h: In function 'xsk_ring_cons__cancel':
>> xsk.h:159:2: error: 'rx' undeclared (first use in this function)
     159 |  rx->cached_cons -= nb;
         |  ^~
   xsk.h:159:2: note: each undeclared identifier is reported only once for each function it appears in
   make[6]: *** [tools/build/Makefile.build:97: kernel/bpf/preload/staticobjs/xsk.o] Error 1
   make[6]: Target '__build' not remade because of errors.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 42936 bytes --]

Re: [PATCH] libbpf: add support for canceling cached_cons advance

[Magnus Karlsson]

On Sun, Nov 22, 2020 at 2:21 PM Li RongQing <lirongqing@baidu.com> wrote:
>
> It is possible to fail receiving packets after calling
> xsk_ring_cons__peek, at this condition, cached_cons has
> been advanced, should be cancelled.

Thanks RongQing,

I have needed this myself in various situations, so I think we should
add this. But your motivation in the commit message is somewhat
confusing. How about something like this?

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.

Replace your commit message with this, fix the bug below, send a v2
and then I am happy to ack this.

/Magnus

> Signed-off-by: Li RongQing <lirongqing@baidu.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 1069c46364ff..4128215c246b 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)
> +{
> +       rx->cached_cons -= nb;

cons-> not rx->. Please make sure the v2 compiles and passes checkpatch.

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

RE: [PATCH] libbpf: add support for canceling cached_cons advance

[Li,Rongqing]

> -----Original Message-----
> From: Magnus Karlsson [mailto:magnus.karlsson@gmail.com]
> Sent: Monday, November 23, 2020 5:40 PM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: Network Development <netdev@vger.kernel.org>; bpf
> <bpf@vger.kernel.org>
> Subject: Re: [PATCH] libbpf: add support for canceling cached_cons advance
> 
> On Sun, Nov 22, 2020 at 2:21 PM Li RongQing <lirongqing@baidu.com> wrote:
> >
> > It is possible to fail receiving packets after calling
> > xsk_ring_cons__peek, at this condition, cached_cons has been advanced,
> > should be cancelled.
> 
> Thanks RongQing,
> 
> I have needed this myself in various situations, so I think we should add this.
> But your motivation in the commit message is somewhat confusing. How about
> something like this?
> 
> 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.
> 
> Replace your commit message with this, fix the bug below, send a v2 and then I
> am happy to ack this.


Thank you very much
> 
> /Magnus
> 
> > Signed-off-by: Li RongQing <lirongqing@baidu.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
> > 1069c46364ff..4128215c246b 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) {
> > +       rx->cached_cons -= nb;
> 
> cons-> not rx->. Please make sure the v2 compiles and passes checkpatch.
> 

Sorry for building error
I will send V2

Thanks 

-Li


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