netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
@ 2023-03-20 10:53 Nuno Gonçalves
  2023-03-20 11:03 ` Leon Romanovsky
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Nuno Gonçalves @ 2023-03-20 10:53 UTC (permalink / raw)
  To: Björn Töpel, Magnus Karlsson, Maciej Fijalkowski,
	Jonathan Lemon, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Christian Brauner, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend
  Cc: Nuno Gonçalves, netdev, bpf, linux-kernel

The remap of fill and completion rings was frowned upon as they
control the usage of UMEM which does not support concurrent use.
At the same time this would disallow the remap of this rings
into another process.

A possible use case is that the user wants to transfer the socket/
UMEM ownerwhip to another process (via SYS_pidfd_getfd) and so
would need to also remap this rings.

This will have no impact on current usages and just relaxes the
remap limitation.

Signed-off-by: Nuno Gonçalves <nunog@fr24.com>
---
 net/xdp/xsk.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 2ac58b282b5eb..2af4ff64b22bd 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1300,10 +1300,11 @@ static int xsk_mmap(struct file *file, struct socket *sock,
 {
 	loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 	unsigned long size = vma->vm_end - vma->vm_start;
+	int state = READ_ONCE(xs->state);
 	struct xdp_sock *xs = xdp_sk(sock->sk);
 	struct xsk_queue *q = NULL;
 
-	if (READ_ONCE(xs->state) != XSK_READY)
+	if (!(state == XSK_READY || state == XSK_BOUND))
 		return -EBUSY;
 
 	if (offset == XDP_PGOFF_RX_RING) {
@@ -1314,9 +1315,11 @@ static int xsk_mmap(struct file *file, struct socket *sock,
 		/* Matches the smp_wmb() in XDP_UMEM_REG */
 		smp_rmb();
 		if (offset == XDP_UMEM_PGOFF_FILL_RING)
-			q = READ_ONCE(xs->fq_tmp);
+			q = READ_ONCE(state == XSK_READY ? xs->fq_tmp :
+							   xs->pool->fq);
 		else if (offset == XDP_UMEM_PGOFF_COMPLETION_RING)
-			q = READ_ONCE(xs->cq_tmp);
+			q = READ_ONCE(state == XSK_READY ? xs->cq_tmp :
+							   xs->pool->cq);
 	}
 
 	if (!q)
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
  2023-03-20 10:53 [PATCH bpf-next] xsk: allow remap of fill and/or completion rings Nuno Gonçalves
@ 2023-03-20 11:03 ` Leon Romanovsky
  2023-03-20 12:27   ` Magnus Karlsson
  2023-03-20 20:24 ` kernel test robot
  2023-03-20 20:34 ` kernel test robot
  2 siblings, 1 reply; 8+ messages in thread
From: Leon Romanovsky @ 2023-03-20 11:03 UTC (permalink / raw)
  To: Nuno Gonçalves
  Cc: Björn Töpel, Magnus Karlsson, Maciej Fijalkowski,
	Jonathan Lemon, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Christian Brauner, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, netdev,
	bpf, linux-kernel

On Mon, Mar 20, 2023 at 10:53:23AM +0000, Nuno Gonçalves wrote:
> The remap of fill and completion rings was frowned upon as they
> control the usage of UMEM which does not support concurrent use.
> At the same time this would disallow the remap of this rings
> into another process.
> 
> A possible use case is that the user wants to transfer the socket/
> UMEM ownerwhip to another process (via SYS_pidfd_getfd) and so
> would need to also remap this rings.
> 
> This will have no impact on current usages and just relaxes the
> remap limitation.
> 
> Signed-off-by: Nuno Gonçalves <nunog@fr24.com>
> ---
>  net/xdp/xsk.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 2ac58b282b5eb..2af4ff64b22bd 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1300,10 +1300,11 @@ static int xsk_mmap(struct file *file, struct socket *sock,
>  {
>  	loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
>  	unsigned long size = vma->vm_end - vma->vm_start;
> +	int state = READ_ONCE(xs->state);
>  	struct xdp_sock *xs = xdp_sk(sock->sk);
>  	struct xsk_queue *q = NULL;
>  
> -	if (READ_ONCE(xs->state) != XSK_READY)
> +	if (!(state == XSK_READY || state == XSK_BOUND))

This if(..) is actually:
 if (state != XSK_READY && state != XSK_BOUND)

Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
  2023-03-20 11:03 ` Leon Romanovsky
@ 2023-03-20 12:27   ` Magnus Karlsson
  2023-03-20 13:40     ` Leon Romanovsky
  0 siblings, 1 reply; 8+ messages in thread
From: Magnus Karlsson @ 2023-03-20 12:27 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Nuno Gonçalves, Björn Töpel, Magnus Karlsson,
	Maciej Fijalkowski, Jonathan Lemon, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Christian Brauner,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, netdev, bpf, linux-kernel

On Mon, 20 Mar 2023 at 12:09, Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Mar 20, 2023 at 10:53:23AM +0000, Nuno Gonçalves wrote:
> > The remap of fill and completion rings was frowned upon as they
> > control the usage of UMEM which does not support concurrent use.
> > At the same time this would disallow the remap of this rings
> > into another process.
> >
> > A possible use case is that the user wants to transfer the socket/
> > UMEM ownerwhip to another process (via SYS_pidfd_getfd) and so

nit: ownership

> > would need to also remap this rings.
> >
> > This will have no impact on current usages and just relaxes the
> > remap limitation.
> >
> > Signed-off-by: Nuno Gonçalves <nunog@fr24.com>
> > ---
> >  net/xdp/xsk.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 2ac58b282b5eb..2af4ff64b22bd 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -1300,10 +1300,11 @@ static int xsk_mmap(struct file *file, struct socket *sock,
> >  {
> >       loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> >       unsigned long size = vma->vm_end - vma->vm_start;
> > +     int state = READ_ONCE(xs->state);

Reverse Christmas Tree notation here please. Move it one line down to
after the *xs declaration.

> >       struct xdp_sock *xs = xdp_sk(sock->sk);
> >       struct xsk_queue *q = NULL;
> >
> > -     if (READ_ONCE(xs->state) != XSK_READY)
> > +     if (!(state == XSK_READY || state == XSK_BOUND))
>
> This if(..) is actually:
>  if (state != XSK_READY && state != XSK_BOUND)

Nuno had it like that to start with when he sent the patch privately
to me, but I responded that I prefered the current one. It is easier
to understand if read out aloud IMO. Do not have any strong feelings
either way since the statements are equivalent.

> Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
  2023-03-20 12:27   ` Magnus Karlsson
@ 2023-03-20 13:40     ` Leon Romanovsky
  2023-03-20 13:45       ` Magnus Karlsson
  0 siblings, 1 reply; 8+ messages in thread
From: Leon Romanovsky @ 2023-03-20 13:40 UTC (permalink / raw)
  To: Magnus Karlsson
  Cc: Nuno Gonçalves, Björn Töpel, Magnus Karlsson,
	Maciej Fijalkowski, Jonathan Lemon, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Christian Brauner,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, netdev, bpf, linux-kernel

On Mon, Mar 20, 2023 at 01:27:18PM +0100, Magnus Karlsson wrote:
> On Mon, 20 Mar 2023 at 12:09, Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Mon, Mar 20, 2023 at 10:53:23AM +0000, Nuno Gonçalves wrote:
> > > The remap of fill and completion rings was frowned upon as they
> > > control the usage of UMEM which does not support concurrent use.
> > > At the same time this would disallow the remap of this rings
> > > into another process.
> > >
> > > A possible use case is that the user wants to transfer the socket/
> > > UMEM ownerwhip to another process (via SYS_pidfd_getfd) and so
> 
> nit: ownership
> 
> > > would need to also remap this rings.
> > >
> > > This will have no impact on current usages and just relaxes the
> > > remap limitation.
> > >
> > > Signed-off-by: Nuno Gonçalves <nunog@fr24.com>
> > > ---
> > >  net/xdp/xsk.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index 2ac58b282b5eb..2af4ff64b22bd 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -1300,10 +1300,11 @@ static int xsk_mmap(struct file *file, struct socket *sock,
> > >  {
> > >       loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> > >       unsigned long size = vma->vm_end - vma->vm_start;
> > > +     int state = READ_ONCE(xs->state);
> 
> Reverse Christmas Tree notation here please. Move it one line down to
> after the *xs declaration.
> 
> > >       struct xdp_sock *xs = xdp_sk(sock->sk);
> > >       struct xsk_queue *q = NULL;
> > >
> > > -     if (READ_ONCE(xs->state) != XSK_READY)
> > > +     if (!(state == XSK_READY || state == XSK_BOUND))
> >
> > This if(..) is actually:
> >  if (state != XSK_READY && state != XSK_BOUND)
> 
> Nuno had it like that to start with when he sent the patch privately
> to me, but I responded that I prefered the current one. It is easier
> to understand if read out aloud IMO. 

"Not equal" is much easier to understand than "not" of whole expression.

> Do not have any strong feelings either way since the statements are equivalent.
> 
> > Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
  2023-03-20 13:40     ` Leon Romanovsky
@ 2023-03-20 13:45       ` Magnus Karlsson
  2023-03-20 15:04         ` Magnus Karlsson
  0 siblings, 1 reply; 8+ messages in thread
From: Magnus Karlsson @ 2023-03-20 13:45 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Nuno Gonçalves, Björn Töpel, Magnus Karlsson,
	Maciej Fijalkowski, Jonathan Lemon, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Christian Brauner,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, netdev, bpf, linux-kernel

On Mon, 20 Mar 2023 at 14:41, Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Mar 20, 2023 at 01:27:18PM +0100, Magnus Karlsson wrote:
> > On Mon, 20 Mar 2023 at 12:09, Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Mon, Mar 20, 2023 at 10:53:23AM +0000, Nuno Gonçalves wrote:
> > > > The remap of fill and completion rings was frowned upon as they
> > > > control the usage of UMEM which does not support concurrent use.
> > > > At the same time this would disallow the remap of this rings
> > > > into another process.
> > > >
> > > > A possible use case is that the user wants to transfer the socket/
> > > > UMEM ownerwhip to another process (via SYS_pidfd_getfd) and so
> >
> > nit: ownership
> >
> > > > would need to also remap this rings.
> > > >
> > > > This will have no impact on current usages and just relaxes the
> > > > remap limitation.
> > > >
> > > > Signed-off-by: Nuno Gonçalves <nunog@fr24.com>
> > > > ---
> > > >  net/xdp/xsk.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > > index 2ac58b282b5eb..2af4ff64b22bd 100644
> > > > --- a/net/xdp/xsk.c
> > > > +++ b/net/xdp/xsk.c
> > > > @@ -1300,10 +1300,11 @@ static int xsk_mmap(struct file *file, struct socket *sock,
> > > >  {
> > > >       loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> > > >       unsigned long size = vma->vm_end - vma->vm_start;
> > > > +     int state = READ_ONCE(xs->state);
> >
> > Reverse Christmas Tree notation here please. Move it one line down to
> > after the *xs declaration.
> >
> > > >       struct xdp_sock *xs = xdp_sk(sock->sk);
> > > >       struct xsk_queue *q = NULL;
> > > >
> > > > -     if (READ_ONCE(xs->state) != XSK_READY)
> > > > +     if (!(state == XSK_READY || state == XSK_BOUND))
> > >
> > > This if(..) is actually:
> > >  if (state != XSK_READY && state != XSK_BOUND)
> >
> > Nuno had it like that to start with when he sent the patch privately
> > to me, but I responded that I prefered the current one. It is easier
> > to understand if read out aloud IMO.
>
> "Not equal" is much easier to understand than "not" of whole expression.

Then my brain is wired differently ;-).

> > Do not have any strong feelings either way since the statements are equivalent.
> >
> > > Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
  2023-03-20 13:45       ` Magnus Karlsson
@ 2023-03-20 15:04         ` Magnus Karlsson
  0 siblings, 0 replies; 8+ messages in thread
From: Magnus Karlsson @ 2023-03-20 15:04 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Nuno Gonçalves, Björn Töpel, Magnus Karlsson,
	Maciej Fijalkowski, Jonathan Lemon, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Christian Brauner,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, netdev, bpf, linux-kernel

On Mon, 20 Mar 2023 at 14:45, Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
>
> On Mon, 20 Mar 2023 at 14:41, Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Mon, Mar 20, 2023 at 01:27:18PM +0100, Magnus Karlsson wrote:
> > > On Mon, 20 Mar 2023 at 12:09, Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Mon, Mar 20, 2023 at 10:53:23AM +0000, Nuno Gonçalves wrote:
> > > > > The remap of fill and completion rings was frowned upon as they
> > > > > control the usage of UMEM which does not support concurrent use.
> > > > > At the same time this would disallow the remap of this rings

these rings

> > > > > into another process.
> > > > >
> > > > > A possible use case is that the user wants to transfer the socket/
> > > > > UMEM ownerwhip to another process (via SYS_pidfd_getfd) and so
> > >
> > > nit: ownership
> > >
> > > > > would need to also remap this rings.

these rings

> > > > >
> > > > > This will have no impact on current usages and just relaxes the
> > > > > remap limitation.
> > > > >
> > > > > Signed-off-by: Nuno Gonçalves <nunog@fr24.com>
> > > > > ---
> > > > >  net/xdp/xsk.c | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > > > index 2ac58b282b5eb..2af4ff64b22bd 100644
> > > > > --- a/net/xdp/xsk.c
> > > > > +++ b/net/xdp/xsk.c
> > > > > @@ -1300,10 +1300,11 @@ static int xsk_mmap(struct file *file, struct socket *sock,
> > > > >  {
> > > > >       loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> > > > >       unsigned long size = vma->vm_end - vma->vm_start;
> > > > > +     int state = READ_ONCE(xs->state);
> > >
> > > Reverse Christmas Tree notation here please. Move it one line down to
> > > after the *xs declaration.
> > >
> > > > >       struct xdp_sock *xs = xdp_sk(sock->sk);
> > > > >       struct xsk_queue *q = NULL;
> > > > >
> > > > > -     if (READ_ONCE(xs->state) != XSK_READY)
> > > > > +     if (!(state == XSK_READY || state == XSK_BOUND))
> > > >
> > > > This if(..) is actually:
> > > >  if (state != XSK_READY && state != XSK_BOUND)
> > >
> > > Nuno had it like that to start with when he sent the patch privately
> > > to me, but I responded that I prefered the current one. It is easier
> > > to understand if read out aloud IMO.
> >
> > "Not equal" is much easier to understand than "not" of whole expression.
>
> Then my brain is wired differently ;-).

Nuno, please prepare a v2 by fixing the now four things above and
reverting this if-expression to what you had before. It is two against
one, so I yield. After that, it is good to go from my point of view.

Thanks!

> > > Do not have any strong feelings either way since the statements are equivalent.
> > >
> > > > Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
  2023-03-20 10:53 [PATCH bpf-next] xsk: allow remap of fill and/or completion rings Nuno Gonçalves
  2023-03-20 11:03 ` Leon Romanovsky
@ 2023-03-20 20:24 ` kernel test robot
  2023-03-20 20:34 ` kernel test robot
  2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-03-20 20:24 UTC (permalink / raw)
  To: Nuno Gonçalves, Björn Töpel, Magnus Karlsson,
	Maciej Fijalkowski, Jonathan Lemon, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Christian Brauner,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend
  Cc: llvm, oe-kbuild-all, netdev, Nuno Gonçalves, bpf,
	linux-kernel

Hi Nuno,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-Gon-alves/xsk-allow-remap-of-fill-and-or-completion-rings/20230320-190022
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230320105323.187307-1-nunog%40fr24.com
patch subject: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
config: i386-randconfig-a004 (https://download.01.org/0day-ci/archive/20230321/202303210417.mv8mDIaV-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/56f6a0c68dd5f4419fd7685cc83ceee2c70f3f2e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nuno-Gon-alves/xsk-allow-remap-of-fill-and-or-completion-rings/20230320-190022
        git checkout 56f6a0c68dd5f4419fd7685cc83ceee2c70f3f2e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303210417.mv8mDIaV-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
           int state = READ_ONCE(xs->state);
                                 ^
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:6: error: initializing 'int' with an expression of incompatible type 'void'
           int state = READ_ONCE(xs->state);
               ^       ~~~~~~~~~~~~~~~~~~~~
>> net/xdp/xsk.c:1318:8: error: cannot take the address of an rvalue of type 'struct xsk_queue *'
                           q = READ_ONCE(state == XSK_READY ? xs->fq_tmp :
                               ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:50:2: note: expanded from macro 'READ_ONCE'
           __READ_ONCE(x);                                                 \
           ^           ~
   include/asm-generic/rwonce.h:44:70: note: expanded from macro '__READ_ONCE'
   #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
                                                                         ^ ~
   net/xdp/xsk.c:1321:8: error: cannot take the address of an rvalue of type 'struct xsk_queue *'
                           q = READ_ONCE(state == XSK_READY ? xs->cq_tmp :
                               ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:50:2: note: expanded from macro 'READ_ONCE'
           __READ_ONCE(x);                                                 \
           ^           ~
   include/asm-generic/rwonce.h:44:70: note: expanded from macro '__READ_ONCE'
   #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
                                                                         ^ ~
   10 errors generated.


vim +/xs +1303 net/xdp/xsk.c

  1297	
  1298	static int xsk_mmap(struct file *file, struct socket *sock,
  1299			    struct vm_area_struct *vma)
  1300	{
  1301		loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
  1302		unsigned long size = vma->vm_end - vma->vm_start;
> 1303		int state = READ_ONCE(xs->state);
  1304		struct xdp_sock *xs = xdp_sk(sock->sk);
  1305		struct xsk_queue *q = NULL;
  1306	
  1307		if (!(state == XSK_READY || state == XSK_BOUND))
  1308			return -EBUSY;
  1309	
  1310		if (offset == XDP_PGOFF_RX_RING) {
  1311			q = READ_ONCE(xs->rx);
  1312		} else if (offset == XDP_PGOFF_TX_RING) {
  1313			q = READ_ONCE(xs->tx);
  1314		} else {
  1315			/* Matches the smp_wmb() in XDP_UMEM_REG */
  1316			smp_rmb();
  1317			if (offset == XDP_UMEM_PGOFF_FILL_RING)
> 1318				q = READ_ONCE(state == XSK_READY ? xs->fq_tmp :
  1319								   xs->pool->fq);
  1320			else if (offset == XDP_UMEM_PGOFF_COMPLETION_RING)
  1321				q = READ_ONCE(state == XSK_READY ? xs->cq_tmp :
  1322								   xs->pool->cq);
  1323		}
  1324	
  1325		if (!q)
  1326			return -EINVAL;
  1327	
  1328		/* Matches the smp_wmb() in xsk_init_queue */
  1329		smp_rmb();
  1330		if (size > q->ring_vmalloc_size)
  1331			return -EINVAL;
  1332	
  1333		return remap_vmalloc_range(vma, q->ring, 0);
  1334	}
  1335	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
  2023-03-20 10:53 [PATCH bpf-next] xsk: allow remap of fill and/or completion rings Nuno Gonçalves
  2023-03-20 11:03 ` Leon Romanovsky
  2023-03-20 20:24 ` kernel test robot
@ 2023-03-20 20:34 ` kernel test robot
  2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-03-20 20:34 UTC (permalink / raw)
  To: Nuno Gonçalves, Björn Töpel, Magnus Karlsson,
	Maciej Fijalkowski, Jonathan Lemon, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Christian Brauner,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend
  Cc: llvm, oe-kbuild-all, netdev, Nuno Gonçalves, bpf,
	linux-kernel

Hi Nuno,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-Gon-alves/xsk-allow-remap-of-fill-and-or-completion-rings/20230320-190022
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230320105323.187307-1-nunog%40fr24.com
patch subject: [PATCH bpf-next] xsk: allow remap of fill and/or completion rings
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20230321/202303210446.mO2yxOwo-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/56f6a0c68dd5f4419fd7685cc83ceee2c70f3f2e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nuno-Gon-alves/xsk-allow-remap-of-fill-and-or-completion-rings/20230320-190022
        git checkout 56f6a0c68dd5f4419fd7685cc83ceee2c70f3f2e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/xdp/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303210446.mO2yxOwo-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
           int state = READ_ONCE(xs->state);
                                 ^
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:24: error: use of undeclared identifier 'xs'
>> net/xdp/xsk.c:1303:6: error: initializing 'int' with an expression of incompatible type 'void'
           int state = READ_ONCE(xs->state);
               ^       ~~~~~~~~~~~~~~~~~~~~
>> net/xdp/xsk.c:1318:8: error: cannot take the address of an rvalue of type 'struct xsk_queue *'
                           q = READ_ONCE(state == XSK_READY ? xs->fq_tmp :
                               ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:50:2: note: expanded from macro 'READ_ONCE'
           __READ_ONCE(x);                                                 \
           ^           ~
   include/asm-generic/rwonce.h:44:70: note: expanded from macro '__READ_ONCE'
   #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
                                                                         ^ ~
   net/xdp/xsk.c:1321:8: error: cannot take the address of an rvalue of type 'struct xsk_queue *'
                           q = READ_ONCE(state == XSK_READY ? xs->cq_tmp :
                               ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:50:2: note: expanded from macro 'READ_ONCE'
           __READ_ONCE(x);                                                 \
           ^           ~
   include/asm-generic/rwonce.h:44:70: note: expanded from macro '__READ_ONCE'
   #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
                                                                         ^ ~
   10 errors generated.


vim +/xs +1303 net/xdp/xsk.c

  1297	
  1298	static int xsk_mmap(struct file *file, struct socket *sock,
  1299			    struct vm_area_struct *vma)
  1300	{
  1301		loff_t offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
  1302		unsigned long size = vma->vm_end - vma->vm_start;
> 1303		int state = READ_ONCE(xs->state);
  1304		struct xdp_sock *xs = xdp_sk(sock->sk);
  1305		struct xsk_queue *q = NULL;
  1306	
  1307		if (!(state == XSK_READY || state == XSK_BOUND))
  1308			return -EBUSY;
  1309	
  1310		if (offset == XDP_PGOFF_RX_RING) {
  1311			q = READ_ONCE(xs->rx);
  1312		} else if (offset == XDP_PGOFF_TX_RING) {
  1313			q = READ_ONCE(xs->tx);
  1314		} else {
  1315			/* Matches the smp_wmb() in XDP_UMEM_REG */
  1316			smp_rmb();
  1317			if (offset == XDP_UMEM_PGOFF_FILL_RING)
> 1318				q = READ_ONCE(state == XSK_READY ? xs->fq_tmp :
  1319								   xs->pool->fq);
  1320			else if (offset == XDP_UMEM_PGOFF_COMPLETION_RING)
  1321				q = READ_ONCE(state == XSK_READY ? xs->cq_tmp :
  1322								   xs->pool->cq);
  1323		}
  1324	
  1325		if (!q)
  1326			return -EINVAL;
  1327	
  1328		/* Matches the smp_wmb() in xsk_init_queue */
  1329		smp_rmb();
  1330		if (size > q->ring_vmalloc_size)
  1331			return -EINVAL;
  1332	
  1333		return remap_vmalloc_range(vma, q->ring, 0);
  1334	}
  1335	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-03-20 20:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-20 10:53 [PATCH bpf-next] xsk: allow remap of fill and/or completion rings Nuno Gonçalves
2023-03-20 11:03 ` Leon Romanovsky
2023-03-20 12:27   ` Magnus Karlsson
2023-03-20 13:40     ` Leon Romanovsky
2023-03-20 13:45       ` Magnus Karlsson
2023-03-20 15:04         ` Magnus Karlsson
2023-03-20 20:24 ` kernel test robot
2023-03-20 20:34 ` kernel test robot

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