Message-ID: <Z7Yld21sv_Ip3gQx@LQ3V64L9R2>
Date: Wed, 19 Feb 2025 13:39:51 -0500
From: Joe Damato <jdamato@...tly.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	shuah@...nel.org, hawk@...nel.org, petrm@...dia.com,
	willemdebruijn.kernel@...il.com
Subject: Re: [PATCH net-next 2/4] selftests: drv-net: add a way to wait for a
 local process

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?

Powered by blists - more mailing lists