[PATCH net-next 0/4] selftests: drv-net: improve the queue test for XSK

[PATCH net-next 0/4] selftests: drv-net: improve the queue test for XSK

[Jakub Kicinski]

We see some flakes in the the XSK test:

   Exception| Traceback (most recent call last):
   Exception|   File "/home/virtme/testing-18/tools/testing/selftests/net/lib/py/ksft.py", line 218, in ksft_run
   Exception|     case(*args)
   Exception|   File "/home/virtme/testing-18/tools/testing/selftests/drivers/net/./queues.py", line 53, in check_xdp
   Exception|     ksft_eq(q['xsk'], {})
   Exception| KeyError: 'xsk'

I think it's because the method or running the helper in the background
is racy. Add more solid infra for waiting for a background helper to be
initialized.

Jakub Kicinski (4):
  selftests: drv-net: use cfg.rpath() in netlink xsk attr test
  selftests: drv-net: add a way to wait for a local process
  selftests: drv-net: improve the use of ksft helpers in XSK queue test
  selftests: drv-net: rename queues check_xdp to check_xsk

 .../selftests/drivers/net/xdp_helper.c        | 22 ++++++-
 tools/testing/selftests/drivers/net/queues.py | 55 ++++++++----------
 tools/testing/selftests/net/lib/py/ksft.py    |  5 ++
 tools/testing/selftests/net/lib/py/utils.py   | 58 +++++++++++++++++--
 4 files changed, 103 insertions(+), 37 deletions(-)

-- 
2.48.1

[PATCH net-next 1/4] selftests: drv-net: use cfg.rpath() in netlink xsk attr test

[Jakub Kicinski]

The cfg.rpath() helper was been recently added to make formatting
paths for helper binaries easier.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/queues.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index 5fdfebc6415f..b6896a57a5fd 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -25,8 +25,7 @@ import subprocess
     return None
 
 def check_xdp(cfg, nl, xdp_queue_id=0) -> None:
-    test_dir = os.path.dirname(os.path.realpath(__file__))
-    xdp = subprocess.Popen([f"{test_dir}/xdp_helper", f"{cfg.ifindex}", f"{xdp_queue_id}"],
+    xdp = subprocess.Popen([cfg.rpath("xdp_helper"), f"{cfg.ifindex}", f"{xdp_queue_id}"],
                            stdin=subprocess.PIPE, stdout=subprocess.PIPE, bufsize=1,
                            text=True)
     defer(xdp.kill)
-- 
2.48.1

Re: [PATCH net-next 1/4] selftests: drv-net: use cfg.rpath() in netlink xsk attr test

[Joe Damato]

On Tue, Feb 18, 2025 at 11:50:45AM -0800, Jakub Kicinski wrote:
> The cfg.rpath() helper was been recently added to make formatting
> paths for helper binaries easier.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/queues.py | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Reviewed-by: Joe Damato <jdamato@fastly.com>

[PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Jakub Kicinski]

We use wait_port_listen() extensively to wait for a process
we spawned to be ready. Not all processes will open listening
sockets. Add a method of explicitly waiting for a child to
be ready. Pass a FD to the spawned process and wait for it
to write a message to us. FD number is passed via KSFT_READY_FD
env variable.

Make use of this method in the queues test to make it less flaky.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/drivers/net/xdp_helper.c        | 22 ++++++-
 tools/testing/selftests/drivers/net/queues.py | 46 ++++++---------
 tools/testing/selftests/net/lib/py/utils.py   | 58 +++++++++++++++++--
 3 files changed, 93 insertions(+), 33 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/drivers/net/xdp_helper.c
index cf06a88b830b..8f77da4f798f 100644
--- a/tools/testing/selftests/drivers/net/xdp_helper.c
+++ b/tools/testing/selftests/drivers/net/xdp_helper.c
@@ -14,6 +14,25 @@
 #define UMEM_SZ (1U << 16)
 #define NUM_DESC (UMEM_SZ / 2048)
 
+/* Move this to a common header when reused! */
+static void ksft_ready(void)
+{
+	const char msg[7] = "ready\n";
+	char *env_str;
+	int fd;
+
+	env_str = getenv("KSFT_READY_FD");
+	if (!env_str)
+		return;
+
+	fd = atoi(env_str);
+	if (!fd)
+		return;
+
+	write(fd, msg, sizeof(msg));
+	close(fd);
+}
+
 /* this is a simple helper program that creates an XDP socket and does the
  * minimum necessary to get bind() to succeed.
  *
@@ -85,8 +104,7 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	/* give the parent program some data when the socket is ready*/
-	fprintf(stdout, "%d\n", sock_fd);
+	ksft_ready();
 
 	/* parent program will write a byte to stdin when its ready for this
 	 * helper to exit
diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index b6896a57a5fd..91e344d108ee 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -5,13 +5,12 @@ from lib.py import ksft_disruptive, ksft_exit, ksft_run
 from lib.py import ksft_eq, ksft_raises, KsftSkipEx, KsftFailEx
 from lib.py import EthtoolFamily, NetdevFamily, NlError
 from lib.py import NetDrvEnv
-from lib.py import cmd, defer, ip
+from lib.py import bkg, cmd, defer, ip
 import errno
 import glob
 import os
 import socket
 import struct
-import subprocess
 
 def sys_get_queues(ifname, qtype='rx') -> int:
     folders = glob.glob(f'/sys/class/net/{ifname}/queues/{qtype}-*')
@@ -25,37 +24,30 @@ import subprocess
     return None
 
 def check_xdp(cfg, nl, xdp_queue_id=0) -> None:
-    xdp = subprocess.Popen([cfg.rpath("xdp_helper"), f"{cfg.ifindex}", f"{xdp_queue_id}"],
-                           stdin=subprocess.PIPE, stdout=subprocess.PIPE, bufsize=1,
-                           text=True)
-    defer(xdp.kill)
+    with bkg(f'{cfg.rpath("xdp_helper")} {cfg.ifindex} {xdp_queue_id}',
+             wait_init=3):
 
-    stdout, stderr = xdp.communicate(timeout=10)
-    rx = tx = False
+        rx = tx = False
 
-    if xdp.returncode == 255:
-        raise KsftSkipEx('AF_XDP unsupported')
-    elif xdp.returncode > 0:
-        raise KsftFailEx('unable to create AF_XDP socket')
+        queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
+        if not queues:
+            raise KsftSkipEx("Netlink reports no queues")
 
-    queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
-    if not queues:
-        raise KsftSkipEx("Netlink reports no queues")
+        for q in queues:
+            if q['id'] == 0:
+                if q['type'] == 'rx':
+                    rx = True
+                if q['type'] == 'tx':
+                    tx = True
 
-    for q in queues:
-        if q['id'] == 0:
-            if q['type'] == 'rx':
-                rx = True
-            if q['type'] == 'tx':
-                tx = True
+                ksft_eq(q['xsk'], {})
+            else:
+                if 'xsk' in q:
+                    _fail("Check failed: xsk attribute set.")
 
-            ksft_eq(q['xsk'], {})
-        else:
-            if 'xsk' in q:
-                _fail("Check failed: xsk attribute set.")
+        ksft_eq(rx, True)
+        ksft_eq(tx, True)
 
-    ksft_eq(rx, True)
-    ksft_eq(tx, True)
 
 def get_queues(cfg, nl) -> None:
     snl = NetdevFamily(recv_size=4096)
diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index 9e3bcddcf3e8..efb9b8fc1447 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -2,8 +2,10 @@
 
 import errno
 import json as _json
+import os
 import random
 import re
+import select
 import socket
 import subprocess
 import time
@@ -15,8 +17,22 @@ import time
         self.cmd = cmd_obj
 
 
+def fd_read_timeout(fd, timeout):
+    rlist, _, _ = select.select([fd], [], [], timeout)
+    if rlist:
+        return os.read(fd, 1024)
+    else:
+        raise TimeoutError("Timeout waiting for fd read")
+
+
 class cmd:
-    def __init__(self, comm, shell=True, fail=True, ns=None, background=False, host=None, timeout=5):
+    """
+    Execute a command on local or remote host.
+
+    Use bkg() instead to run a command in the background.
+    """
+    def __init__(self, comm, shell=True, fail=True, ns=None, background=False,
+                 host=None, timeout=5, wait_init=None):
         if ns:
             comm = f'ip netns exec {ns} ' + comm
 
@@ -28,8 +44,23 @@ import time
         if host:
             self.proc = host.cmd(comm)
         else:
+            # wait_init lets us wait for the background process to fully start,
+            # we pass an FD to the child process, and wait for it to write back
+            pass_fds = ()
+            env = os.environ.copy()
+            if wait_init is not None:
+                rfd, wfd = os.pipe()
+                pass_fds = (wfd, )
+                env["KSFT_READY_FD"] = str(wfd)
             self.proc = subprocess.Popen(comm, shell=shell, stdout=subprocess.PIPE,
-                                         stderr=subprocess.PIPE)
+                                         stderr=subprocess.PIPE, pass_fds=pass_fds,
+                                         env=env)
+            if wait_init is not None:
+                os.close(wfd)
+                msg = fd_read_timeout(rfd, wait_init)
+                os.close(rfd)
+                if not msg:
+                    raise Exception("Did not receive ready message")
         if not background:
             self.process(terminate=False, fail=fail, timeout=timeout)
 
@@ -54,10 +85,29 @@ import time
 
 
 class bkg(cmd):
+    """
+    Run a command in the background.
+
+    Examples usage:
+
+    Run a command on remote host, and wait for it to finish.
+    This is usually paired with wait_port_listen() to make sure
+    the command has initialized:
+
+        with bkg("socat ...", exit_wait=True, host=cfg.remote) as nc:
+            ...
+
+    Run a command and expect it to let us know that it's ready
+    by writing to a special file decriptor passed via KSFT_READY_FD.
+    Command will be terminated when we exit the context manager:
+
+        with bkg("my_binary", wait_init=5):
+    """
     def __init__(self, comm, shell=True, fail=None, ns=None, host=None,
-                 exit_wait=False):
+                 exit_wait=False, wait_init=None):
         super().__init__(comm, background=True,
-                         shell=shell, fail=fail, ns=ns, host=host)
+                         shell=shell, fail=fail, ns=ns, host=host,
+                         wait_init=wait_init)
         self.terminate = not exit_wait
         self.check_fail = fail
 
-- 
2.48.1

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Stanislav Fomichev]

On 02/18, Jakub Kicinski wrote:
> We use wait_port_listen() extensively to wait for a process
> we spawned to be ready. Not all processes will open listening
> sockets. Add a method of explicitly waiting for a child to
> be ready. Pass a FD to the spawned process and wait for it
> to write a message to us. FD number is passed via KSFT_READY_FD
> env variable.
> 
> Make use of this method in the queues test to make it less flaky.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../selftests/drivers/net/xdp_helper.c        | 22 ++++++-
>  tools/testing/selftests/drivers/net/queues.py | 46 ++++++---------
>  tools/testing/selftests/net/lib/py/utils.py   | 58 +++++++++++++++++--
>  3 files changed, 93 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/drivers/net/xdp_helper.c
> index cf06a88b830b..8f77da4f798f 100644
> --- a/tools/testing/selftests/drivers/net/xdp_helper.c
> +++ b/tools/testing/selftests/drivers/net/xdp_helper.c
> @@ -14,6 +14,25 @@
>  #define UMEM_SZ (1U << 16)
>  #define NUM_DESC (UMEM_SZ / 2048)
>  
> +/* Move this to a common header when reused! */
> +static void ksft_ready(void)
> +{
> +	const char msg[7] = "ready\n";
> +	char *env_str;
> +	int fd;

[..]

> +	env_str = getenv("KSFT_READY_FD");
> +	if (!env_str)
> +		return;
> +
> +	fd = atoi(env_str);
> +	if (!fd)
> +		return;

optional nit: should these fail with error() instead of silent return?
Should guarantee that the caller is doing everything correctly.
(passing wait_init vs waiting for a port)

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Jakub Kicinski]

On Tue, 18 Feb 2025 13:10:14 -0800 Stanislav Fomichev wrote:
> > +	env_str = getenv("KSFT_READY_FD");
> > +	if (!env_str)
> > +		return;
> > +
> > +	fd = atoi(env_str);
> > +	if (!fd)
> > +		return;  
> 
> optional nit: should these fail with error() instead of silent return?
> Should guarantee that the caller is doing everything correctly.
> (passing wait_init vs waiting for a port)

My thinking was that you may want to run the helper manually during
development and then the env variable won't be set.

Given that we currently don't expose the stdout/stderr if wait fails
adding a print seemed a little performative..  We need to go back
and provide better support for debug (/verbose output) at some stage.

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Stanislav Fomichev]

On 02/18, Jakub Kicinski wrote:
> On Tue, 18 Feb 2025 13:10:14 -0800 Stanislav Fomichev wrote:
> > > +	env_str = getenv("KSFT_READY_FD");
> > > +	if (!env_str)
> > > +		return;
> > > +
> > > +	fd = atoi(env_str);
> > > +	if (!fd)
> > > +		return;  
> > 
> > optional nit: should these fail with error() instead of silent return?
> > Should guarantee that the caller is doing everything correctly.
> > (passing wait_init vs waiting for a port)
> 
> My thinking was that you may want to run the helper manually during
> development and then the env variable won't be set.
> 
> Given that we currently don't expose the stdout/stderr if wait fails
> adding a print seemed a little performative..  We need to go back
> and provide better support for debug (/verbose output) at some stage.

Good point. In this case, we can fail only on the first case (!env_str);
should still catch the wrong invocation and let you do KSFT_READY_FD= ./helper
manual run. But agreed, that not worth the respin..

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Joe Damato]

On Tue, Feb 18, 2025 at 11:50:46AM -0800, Jakub Kicinski wrote:
> We use wait_port_listen() extensively to wait for a process
> we spawned to be ready. Not all processes will open listening
> sockets. Add a method of explicitly waiting for a child to
> be ready. Pass a FD to the spawned process and wait for it
> to write a message to us. FD number is passed via KSFT_READY_FD
> env variable.
> 
> Make use of this method in the queues test to make it less flaky.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../selftests/drivers/net/xdp_helper.c        | 22 ++++++-
>  tools/testing/selftests/drivers/net/queues.py | 46 ++++++---------
>  tools/testing/selftests/net/lib/py/utils.py   | 58 +++++++++++++++++--
>  3 files changed, 93 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/drivers/net/xdp_helper.c
> index cf06a88b830b..8f77da4f798f 100644
> --- a/tools/testing/selftests/drivers/net/xdp_helper.c
> +++ b/tools/testing/selftests/drivers/net/xdp_helper.c
> @@ -14,6 +14,25 @@
>  #define UMEM_SZ (1U << 16)
>  #define NUM_DESC (UMEM_SZ / 2048)
>  
> +/* Move this to a common header when reused! */
> +static void ksft_ready(void)
> +{
> +	const char msg[7] = "ready\n";
> +	char *env_str;
> +	int fd;
> +
> +	env_str = getenv("KSFT_READY_FD");
> +	if (!env_str)
> +		return;
> +
> +	fd = atoi(env_str);
> +	if (!fd)
> +		return;
> +
> +	write(fd, msg, sizeof(msg));
> +	close(fd);
> +}
> +
>  /* this is a simple helper program that creates an XDP socket and does the
>   * minimum necessary to get bind() to succeed.
>   *
> @@ -85,8 +104,7 @@ int main(int argc, char **argv)
>  		return 1;
>  	}
>  
> -	/* give the parent program some data when the socket is ready*/
> -	fprintf(stdout, "%d\n", sock_fd);
> +	ksft_ready();
>  
>  	/* parent program will write a byte to stdin when its ready for this
>  	 * helper to exit
> diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
> index b6896a57a5fd..91e344d108ee 100755
> --- a/tools/testing/selftests/drivers/net/queues.py
> +++ b/tools/testing/selftests/drivers/net/queues.py
> @@ -5,13 +5,12 @@ from lib.py import ksft_disruptive, ksft_exit, ksft_run
>  from lib.py import ksft_eq, ksft_raises, KsftSkipEx, KsftFailEx
>  from lib.py import EthtoolFamily, NetdevFamily, NlError
>  from lib.py import NetDrvEnv
> -from lib.py import cmd, defer, ip
> +from lib.py import bkg, cmd, defer, ip
>  import errno
>  import glob
>  import os
>  import socket
>  import struct
> -import subprocess
>  
>  def sys_get_queues(ifname, qtype='rx') -> int:
>      folders = glob.glob(f'/sys/class/net/{ifname}/queues/{qtype}-*')
> @@ -25,37 +24,30 @@ import subprocess
>      return None
>  
>  def check_xdp(cfg, nl, xdp_queue_id=0) -> None:
> -    xdp = subprocess.Popen([cfg.rpath("xdp_helper"), f"{cfg.ifindex}", f"{xdp_queue_id}"],
> -                           stdin=subprocess.PIPE, stdout=subprocess.PIPE, bufsize=1,
> -                           text=True)
> -    defer(xdp.kill)
> +    with bkg(f'{cfg.rpath("xdp_helper")} {cfg.ifindex} {xdp_queue_id}',
> +             wait_init=3):
>  
> -    stdout, stderr = xdp.communicate(timeout=10)
> -    rx = tx = False
> +        rx = tx = False
>  
> -    if xdp.returncode == 255:
> -        raise KsftSkipEx('AF_XDP unsupported')
> -    elif xdp.returncode > 0:

Removing this check causes a stack trace on my XDP-disabled kernel,
whereas with the existing code it caused a skip.

Maybe that's OK, though?

The issue is that xdp_helper.c fails and exits with return -1 before
the call to ksft_ready() which results in the following:

# Exception| Traceback (most recent call last):
# Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/ksft.py", line 223, in ksft_run
# Exception|     case(*args)
# Exception|   File "/home/jdamato/code/net-next/./tools/testing/selftests/drivers/net/queues.py", line 27, in check_xsk
# Exception|     with bkg(f'{cfg.rpath("xdp_helper")} {cfg.ifindex} {xdp_queue_id}',
# Exception|          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/utils.py", line 108, in __init__
# Exception|     super().__init__(comm, background=True,
# Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/utils.py", line 63, in __init__
# Exception|     raise Exception("Did not receive ready message")
# Exception| Exception: Did not receive ready message
not ok 4 queues.check_xsk
# Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0

I had originally modified the test so that if XDP is disabled in the
kernel it would skip, but I think you mentioned in a previous thread
that this was a "non-goal", IIRC ?

No strong opinion on my side as to what the behavior should be when
XDP is disabled, but wanted to mention this so that the behavior
change was known.

Separately: I retested this on a machine with XDP enabled, both with
and without NETIF set and the test seems to hang because the helper
is blocked on:

read(STDIN_FILENO, &byte, 1);

according to strace:

strace: Process 14198 attached
21:50:02 read(0,

So, I think this patch needs to be tweaked to write a byte to the
helper so it exits (I assume before the defer was killing it?) or
the helper needs to be modified in way?

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Jakub Kicinski]

On Tue, 18 Feb 2025 16:52:39 -0500 Joe Damato wrote:
> Removing this check causes a stack trace on my XDP-disabled kernel,
> whereas with the existing code it caused a skip.
> 
> Maybe that's OK, though?
> 
> The issue is that xdp_helper.c fails and exits with return -1 before
> the call to ksft_ready() which results in the following:
> 
> # Exception| Traceback (most recent call last):
> # Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/ksft.py", line 223, in ksft_run
> # Exception|     case(*args)
> # Exception|   File "/home/jdamato/code/net-next/./tools/testing/selftests/drivers/net/queues.py", line 27, in check_xsk
> # Exception|     with bkg(f'{cfg.rpath("xdp_helper")} {cfg.ifindex} {xdp_queue_id}',
> # Exception|          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/utils.py", line 108, in __init__
> # Exception|     super().__init__(comm, background=True,
> # Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/utils.py", line 63, in __init__
> # Exception|     raise Exception("Did not receive ready message")
> # Exception| Exception: Did not receive ready message
> not ok 4 queues.check_xsk
> # Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0
> 
> I had originally modified the test so that if XDP is disabled in the
> kernel it would skip, but I think you mentioned in a previous thread
> that this was a "non-goal", IIRC ?
> 
> No strong opinion on my side as to what the behavior should be when
> XDP is disabled, but wanted to mention this so that the behavior
> change was known.

I thought of doing this:

diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/drivers/net/xdp_helper.c
index 8f77da4f798f..8c34e8915fc4 100644
--- a/tools/testing/selftests/drivers/net/xdp_helper.c
+++ b/tools/testing/selftests/drivers/net/xdp_helper.c
@@ -53,7 +53,7 @@ int main(int argc, char **argv)
        int queue;
        char byte;
 
-       if (argc != 3) {
+       if (argc > 1 && argc != 3) {
                fprintf(stderr, "Usage: %s ifindex queue_id", argv[0]);
                return 1;
        }
@@ -69,6 +69,13 @@ int main(int argc, char **argv)
                return 1;
        }
 
+
+       if (argc == 1) {
+               printf("AF_XDP support detected\n");
+               close(sock_fd);
+               return 0;
+       }
+
        ifindex = atoi(argv[1]);
        queue = atoi(argv[2]);
 

Then we can run the helper with no arguments, just to check if af_xdp
is supported. If that returns 0 we go on, otherwise we print your nice
error.

LMK if that sounds good, assuming a respin is needed I can add that :)

> Separately: I retested this on a machine with XDP enabled, both with
> and without NETIF set and the test seems to hang because the helper
> is blocked on:
> 
> read(STDIN_FILENO, &byte, 1);
> 
> according to strace:
> 
> strace: Process 14198 attached
> 21:50:02 read(0,
> 
> So, I think this patch needs to be tweaked to write a byte to the
> helper so it exits (I assume before the defer was killing it?) or
> the helper needs to be modified in way?

What Python version do you have? 

For me the xdp process doesn't wait at all. Running this under vng 
and Python 3.13 the read returns 0 immediately.

Even if it doesn't we run bkg() with default params, so exit_wait=False
init will set:

	self.terminate = not exit_wait

and then __exit__ will do:

	return self.process(terminate=self.terminate, fail=self.check_fail)

which does:

	if self.terminate:
		self.proc.terminate()

so the helper should get a SIGINT, no?

We shall find out if NIPA agrees with my local system at 4p.

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Jakub Kicinski]

On Tue, 18 Feb 2025 15:05:12 -0800 Jakub Kicinski wrote:
> We shall find out if NIPA agrees with my local system at 4p.

NIPA agrees with you, I'll take another look tomorrow.
-- 
pw-bot: cr

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Joe Damato]

On Tue, Feb 18, 2025 at 05:37:39PM -0800, Jakub Kicinski wrote:
> On Tue, 18 Feb 2025 15:05:12 -0800 Jakub Kicinski wrote:
> > We shall find out if NIPA agrees with my local system at 4p.
> 
> NIPA agrees with you, I'll take another look tomorrow.

I think I debugged why it happens on my system, hopefully you see
that message before you go through your own debug cycle :)

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Joe Damato]

On Tue, Feb 18, 2025 at 03:05:12PM -0800, Jakub Kicinski wrote:
> On Tue, 18 Feb 2025 16:52:39 -0500 Joe Damato wrote:
> > Removing this check causes a stack trace on my XDP-disabled kernel,
> > whereas with the existing code it caused a skip.
> > 
> > Maybe that's OK, though?
> > 
> > The issue is that xdp_helper.c fails and exits with return -1 before
> > the call to ksft_ready() which results in the following:
> > 
> > # Exception| Traceback (most recent call last):
> > # Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/ksft.py", line 223, in ksft_run
> > # Exception|     case(*args)
> > # Exception|   File "/home/jdamato/code/net-next/./tools/testing/selftests/drivers/net/queues.py", line 27, in check_xsk
> > # Exception|     with bkg(f'{cfg.rpath("xdp_helper")} {cfg.ifindex} {xdp_queue_id}',
> > # Exception|          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > # Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/utils.py", line 108, in __init__
> > # Exception|     super().__init__(comm, background=True,
> > # Exception|   File "/home/jdamato/code/net-next/tools/testing/selftests/net/lib/py/utils.py", line 63, in __init__
> > # Exception|     raise Exception("Did not receive ready message")
> > # Exception| Exception: Did not receive ready message
> > not ok 4 queues.check_xsk
> > # Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0
> > 
> > I had originally modified the test so that if XDP is disabled in the
> > kernel it would skip, but I think you mentioned in a previous thread
> > that this was a "non-goal", IIRC ?
> > 
> > No strong opinion on my side as to what the behavior should be when
> > XDP is disabled, but wanted to mention this so that the behavior
> > change was known.
> 
> I thought of doing this:
> 
> diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/drivers/net/xdp_helper.c
> index 8f77da4f798f..8c34e8915fc4 100644
> --- a/tools/testing/selftests/drivers/net/xdp_helper.c
> +++ b/tools/testing/selftests/drivers/net/xdp_helper.c
> @@ -53,7 +53,7 @@ int main(int argc, char **argv)
>         int queue;
>         char byte;
>  
> -       if (argc != 3) {
> +       if (argc > 1 && argc != 3) {
>                 fprintf(stderr, "Usage: %s ifindex queue_id", argv[0]);
>                 return 1;
>         }
> @@ -69,6 +69,13 @@ int main(int argc, char **argv)
>                 return 1;
>         }
>  
> +
> +       if (argc == 1) {
> +               printf("AF_XDP support detected\n");
> +               close(sock_fd);
> +               return 0;
> +       }
> +
>         ifindex = atoi(argv[1]);
>         queue = atoi(argv[2]);
>  
> 
> Then we can run the helper with no arguments, just to check if af_xdp
> is supported. If that returns 0 we go on, otherwise we print your nice
> error.
> 
> LMK if that sounds good, assuming a respin is needed I can add that :)

That seems to fine; if you do decide to go this route in a re-spin,
would you mind also adding a "\n" in the fprintf after "ifindex
queue_id" ? Sorry I missed that on my initial implementation.

That missing \n was mentioned by Kurt in another thread:

https://lore.kernel.org/netdev/878qq22xk3.fsf@kurt.kurt.home/T/#m4d0778b9e849bc72064074105182f4c84ba55eb2
 
> > Separately: I retested this on a machine with XDP enabled, both with
> > and without NETIF set and the test seems to hang because the helper
> > is blocked on:
> > 
> > read(STDIN_FILENO, &byte, 1);
> > 
> > according to strace:
> > 
> > strace: Process 14198 attached
> > 21:50:02 read(0,
> > 
> > So, I think this patch needs to be tweaked to write a byte to the
> > helper so it exits (I assume before the defer was killing it?) or
> > the helper needs to be modified in way?
> 
> What Python version do you have? 

Python 3.12.3 (via Ubuntu 24.04.1 LTS).

I can re-test with a different version using pyenv if you'd like?

Are there docs which mention which python version tests should be
compatible with? If so, could you pass along a link? Sorry if I
missed that.

> 
> For me the xdp process doesn't wait at all. Running this under vng 
> and Python 3.13 the read returns 0 immediately.

Interesting, so something changed between 3.12.3 and 3.13, I
suppose.
 
> Even if it doesn't we run bkg() with default params, so exit_wait=False
> init will set:
> 
> 	self.terminate = not exit_wait
> 
> and then __exit__ will do:
> 
> 	return self.process(terminate=self.terminate, fail=self.check_fail)
> 
> which does:
> 
> 	if self.terminate:
> 		self.proc.terminate()
> 
> so the helper should get a SIGINT, no?

(Minor nit, the python docs say terminate delivers a SIGTERM, but
please continue reading)

I'm not a python programmer but did try a bit to debug this. Looking
at strace on my system, it seems that the script forks twice and the
SIGTERM is sent to the wrong pid?

I suppose it forks twice because the helper is invoked via /bin/sh:

18:27:15 vfork(strace: Process 448303 attached
 <unfinished ...>

So, pid 448303 is the first forked process, which does an execve:

[pid 448303] 18:27:15 execve("/bin/sh", ["/bin/sh", "-c", "/home/jdamato/code/net-next/tools/testing/selftests/drivers/net/xdp_helper 5 0"], 0x7f661511fad0 /* 26 vars */ <unfinished ...>

So /bin/sh runs and that then forks again to run the helper:

[pid 448278] 18:27:15 <... vfork resumed>) = 448303

[pid 448303] 18:27:15 vfork(strace: Process 448304 attached
 <unfinished ...>

[pid 448304] 18:27:15 execve("/home/jdamato/code/net-next/tools/testing/selftests/drivers/net/xdp_helper", ["/home/jdamato/code/net-next/tools/testing/selftests/drivers/net/xdp_helper", "5", "0"], 0x55faf4f81918 /* 26 vars */ <unfinished ...>

So pid 448304 is the process doing the execve to run xdp_helper and
would need to be the one getting the SIGTERM/SIGINT

We can see it does the read:

[pid 448304] 18:27:15 read(0,  <unfinished ...>

However, later the python script does the terminate flow you've
described in your message but sends the signal to the /bin/sh that
was forked off:

[pid 448278] 18:27:15 kill(448303, SIGTERM) = 0
[...]
[pid 448303] 18:27:15 +++ killed by SIGTERM +++

But pid 448304 is xdp_helper, which is still running and should be
the one to get the TERM.

I have no idea why this would be different on your system vs mine.
Maybe something changed with Python between Python versions?

> We shall find out if NIPA agrees with my local system at 4p.

Sorry for the noob question, but is there a NIPA url or something I
can look at to see if this worked / if future tests I submit work?

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Jakub Kicinski]

On Wed, 19 Feb 2025 13:39:51 -0500 Joe Damato wrote:
> On Tue, Feb 18, 2025 at 03:05:12PM -0800, Jakub Kicinski wrote:
> > On Tue, 18 Feb 2025 16:52:39 -0500 Joe Damato wrote:  
> > Then we can run the helper with no arguments, just to check if af_xdp
> > is supported. If that returns 0 we go on, otherwise we print your nice
> > error.
> > 
> > LMK if that sounds good, assuming a respin is needed I can add that :)  
> 
> That seems to fine; if you do decide to go this route in a re-spin,
> would you mind also adding a "\n" in the fprintf after "ifindex
> queue_id" ? Sorry I missed that on my initial implementation.
> 
> That missing \n was mentioned by Kurt in another thread:

Will do!

> > > Separately: I retested this on a machine with XDP enabled, both with
> > > and without NETIF set and the test seems to hang because the helper
> > > is blocked on:
> > > 
> > > read(STDIN_FILENO, &byte, 1);
> > > 
> > > according to strace:
> > > 
> > > strace: Process 14198 attached
> > > 21:50:02 read(0,
> > > 
> > > So, I think this patch needs to be tweaked to write a byte to the
> > > helper so it exits (I assume before the defer was killing it?) or
> > > the helper needs to be modified in way?  
> > 
> > What Python version do you have?   
> 
> Python 3.12.3 (via Ubuntu 24.04.1 LTS).
> 
> I can re-test with a different version using pyenv if you'd like?
> 
> Are there docs which mention which python version tests should be
> compatible with? If so, could you pass along a link? Sorry if I
> missed that.

You're good, sorry, tests are supposed to run on any non-EOL
version of python.

<snip>
> [pid 448278] 18:27:15 kill(448303, SIGTERM) = 0
> [...]
> [pid 448303] 18:27:15 +++ killed by SIGTERM +++
> 
> But pid 448304 is xdp_helper, which is still running and should be
> the one to get the TERM.

Very interesting. I dug deeper into this, and it turns out its shell
dependent. I'm guessing you're using one of the cool shells, I use
bash. bash does a direct exec for "sh -c X", other shells fork first.

I'll add a warning in bkg() for combining shell=True and terminate=True.

> I have no idea why this would be different on your system vs mine.
> Maybe something changed with Python between Python versions?

More digging still necessary here, as NIPA also runs on bash.
So your problem is different than NIPA's.
NIPA runs:

  make -C tools/testing/selftests TARGETS="drivers/net" \
	  TEST_PROGS=queues.py TEST_GEN_PROGS="" run_tests

which runs thru a layer of perl for output prefixing:

	tools/testing/selftests/kselftest/prefix.pl

which in turn send a SIGTTIN when we call read(), and hangs the helper.

> > We shall find out if NIPA agrees with my local system at 4p.  
> 
> Sorry for the noob question, but is there a NIPA url or something I
> can look at to see if this worked / if future tests I submit work?

https://netdev.bots.linux.dev/status.html

With the disclaimer that we discourage people from looking at it. 
It tests everything on  the list combined, we can't support the
corporate "try until CI is green" development model. Not that 
I'd accuse you of such practices :)

Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a local process

[Joe Damato]

On Wed, Feb 19, 2025 at 02:48:44PM -0800, Jakub Kicinski wrote:
> On Wed, 19 Feb 2025 13:39:51 -0500 Joe Damato wrote:
> > On Tue, Feb 18, 2025 at 03:05:12PM -0800, Jakub Kicinski wrote:
> > > On Tue, 18 Feb 2025 16:52:39 -0500 Joe Damato wrote:  

[...]

> <snip>
> > [pid 448278] 18:27:15 kill(448303, SIGTERM) = 0
> > [...]
> > [pid 448303] 18:27:15 +++ killed by SIGTERM +++
> > 
> > But pid 448304 is xdp_helper, which is still running and should be
> > the one to get the TERM.
> 
> Very interesting. I dug deeper into this, and it turns out its shell
> dependent. I'm guessing you're using one of the cool shells, I use
> bash. bash does a direct exec for "sh -c X", other shells fork first.

I am using bash, as well. The version comes with Ubuntu 24.04.1:

GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu)

I wonder if it's something about the environment that causes bash to
act this on my machine and NIPA but not yours?

> I'll add a warning in bkg() for combining shell=True and terminate=True.
> 
> > I have no idea why this would be different on your system vs mine.
> > Maybe something changed with Python between Python versions?
> 
> More digging still necessary here, as NIPA also runs on bash.
> So your problem is different than NIPA's.
> NIPA runs:
> 
>   make -C tools/testing/selftests TARGETS="drivers/net" \
> 	  TEST_PROGS=queues.py TEST_GEN_PROGS="" run_tests
> 
> which runs thru a layer of perl for output prefixing:
> 
> 	tools/testing/selftests/kselftest/prefix.pl
> 
> which in turn send a SIGTTIN when we call read(), and hangs the helper.
> 
> > > We shall find out if NIPA agrees with my local system at 4p.  
> > 
> > Sorry for the noob question, but is there a NIPA url or something I
> > can look at to see if this worked / if future tests I submit work?
> 
> https://netdev.bots.linux.dev/status.html
> 
> With the disclaimer that we discourage people from looking at it. 
> It tests everything on  the list combined, we can't support the
> corporate "try until CI is green" development model. Not that 
> I'd accuse you of such practices :)

Yea I wasn't sure what the right process is for selftests; the ones
I've hacked on or written I've run locally until they passed, but I
suppose it is possible that in cases like this upstream CI will fail
for some reason or another.

[PATCH net-next 3/4] selftests: drv-net: improve the use of ksft helpers in XSK queue test

[Jakub Kicinski]

Avoid exceptions when xsk attr is not present, and add a proper ksft
helper for "not in" condition.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/queues.py | 9 +++++----
 tools/testing/selftests/net/lib/py/ksft.py    | 5 +++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index 91e344d108ee..3cbcbaf5eaeb 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -2,7 +2,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 from lib.py import ksft_disruptive, ksft_exit, ksft_run
-from lib.py import ksft_eq, ksft_raises, KsftSkipEx, KsftFailEx
+from lib.py import ksft_eq, ksft_not_in, ksft_raises, KsftSkipEx, KsftFailEx
 from lib.py import EthtoolFamily, NetdevFamily, NlError
 from lib.py import NetDrvEnv
 from lib.py import bkg, cmd, defer, ip
@@ -40,10 +40,11 @@ import struct
                 if q['type'] == 'tx':
                     tx = True
 
-                ksft_eq(q['xsk'], {})
+                ksft_eq(q.get('xsk', None), {},
+                        comment="xsk attr on queue we configured")
             else:
-                if 'xsk' in q:
-                    _fail("Check failed: xsk attribute set.")
+                ksft_not_in('xsk', q,
+                            comment="xsk attr on queue we didn't configure")
 
         ksft_eq(rx, True)
         ksft_eq(tx, True)
diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index 3efe005436cd..fd23349fa8ca 100644
--- a/tools/testing/selftests/net/lib/py/ksft.py
+++ b/tools/testing/selftests/net/lib/py/ksft.py
@@ -71,6 +71,11 @@ KSFT_DISRUPTIVE = True
         _fail("Check failed", a, "not in", b, comment)
 
 
+def ksft_not_in(a, b, comment=""):
+    if a in b:
+        _fail("Check failed", a, "in", b, comment)
+
+
 def ksft_is(a, b, comment=""):
     if a is not b:
         _fail("Check failed", a, "is not", b, comment)
-- 
2.48.1

Re: [PATCH net-next 3/4] selftests: drv-net: improve the use of ksft helpers in XSK queue test

[Joe Damato]

On Tue, Feb 18, 2025 at 11:50:47AM -0800, Jakub Kicinski wrote:
> Avoid exceptions when xsk attr is not present, and add a proper ksft
> helper for "not in" condition.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/queues.py | 9 +++++----
>  tools/testing/selftests/net/lib/py/ksft.py    | 5 +++++
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 

Reviewed-by: Joe Damato <jdamato@fastly.com>

[PATCH net-next 4/4] selftests: drv-net: rename queues check_xdp to check_xsk

[Jakub Kicinski]

The test is for AF_XDP, we refer to AF_XDP as XSK.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/queues.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index 3cbcbaf5eaeb..feff4d475ef8 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -23,7 +23,7 @@ import struct
         return len([q for q in queues if q['type'] == qtype])
     return None
 
-def check_xdp(cfg, nl, xdp_queue_id=0) -> None:
+def check_xsk(cfg, nl, xdp_queue_id=0) -> None:
     with bkg(f'{cfg.rpath("xdp_helper")} {cfg.ifindex} {xdp_queue_id}',
              wait_init=3):
 
@@ -109,7 +109,8 @@ import struct
 
 def main() -> None:
     with NetDrvEnv(__file__, queue_count=100) as cfg:
-        ksft_run([get_queues, addremove_queues, check_down, check_xdp], args=(cfg, NetdevFamily()))
+        ksft_run([get_queues, addremove_queues, check_down, check_xsk],
+                 args=(cfg, NetdevFamily()))
     ksft_exit()
 
 
-- 
2.48.1

Re: [PATCH net-next 4/4] selftests: drv-net: rename queues check_xdp to check_xsk

[Joe Damato]

On Tue, Feb 18, 2025 at 11:50:48AM -0800, Jakub Kicinski wrote:
> The test is for AF_XDP, we refer to AF_XDP as XSK.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/queues.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next 0/4] selftests: drv-net: improve the queue test for XSK

[Stanislav Fomichev]

On 02/18, Jakub Kicinski wrote:
> We see some flakes in the the XSK test:
> 
>    Exception| Traceback (most recent call last):
>    Exception|   File "/home/virtme/testing-18/tools/testing/selftests/net/lib/py/ksft.py", line 218, in ksft_run
>    Exception|     case(*args)
>    Exception|   File "/home/virtme/testing-18/tools/testing/selftests/drivers/net/./queues.py", line 53, in check_xdp
>    Exception|     ksft_eq(q['xsk'], {})
>    Exception| KeyError: 'xsk'
> 
> I think it's because the method or running the helper in the background
> is racy. Add more solid infra for waiting for a background helper to be
> initialized.
> 
> Jakub Kicinski (4):
>   selftests: drv-net: use cfg.rpath() in netlink xsk attr test
>   selftests: drv-net: add a way to wait for a local process
>   selftests: drv-net: improve the use of ksft helpers in XSK queue test
>   selftests: drv-net: rename queues check_xdp to check_xsk
> 
>  .../selftests/drivers/net/xdp_helper.c        | 22 ++++++-
>  tools/testing/selftests/drivers/net/queues.py | 55 ++++++++----------
>  tools/testing/selftests/net/lib/py/ksft.py    |  5 ++
>  tools/testing/selftests/net/lib/py/utils.py   | 58 +++++++++++++++++--
>  4 files changed, 103 insertions(+), 37 deletions(-)
> 
> -- 
> 2.48.1
> 

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 0/4] selftests: drv-net: improve the queue test for XSK

[Joe Damato]

On Tue, Feb 18, 2025 at 11:50:44AM -0800, Jakub Kicinski wrote:
> We see some flakes in the the XSK test:
> 
>    Exception| Traceback (most recent call last):
>    Exception|   File "/home/virtme/testing-18/tools/testing/selftests/net/lib/py/ksft.py", line 218, in ksft_run
>    Exception|     case(*args)
>    Exception|   File "/home/virtme/testing-18/tools/testing/selftests/drivers/net/./queues.py", line 53, in check_xdp
>    Exception|     ksft_eq(q['xsk'], {})
>    Exception| KeyError: 'xsk'
> 
> I think it's because the method or running the helper in the background
> is racy. Add more solid infra for waiting for a background helper to be
> initialized.

Sorry for the raciness I introduced. I think overall the direction
this is going is good - thanks for cleaning this up.

Please see my comments on patch 2 and the hang introduced with this
change.