[RFC net-next 0/2] netdevgenl: Add an xsk attribute to queues

[RFC net-next 0/2] netdevgenl: Add an xsk attribute to queues

[Joe Damato]

Greetings:

This is an attempt to followup on something Jakub asked me about [1],
adding an xsk attribute to queues and more clearly documenting which
queues are linked to NAPIs...

But:

1. I couldn't pick a good "thing" to expose as "xsk", so I chose 0 or 1.
   Happy to take suggestions on what might be better to expose for the
   xsk queue attribute.

2. I create a silly C helper program to create an XDP socket in order to
   add a new test to queues.py. I'm not particularly good at python
   programming, so there's probably a better way to do this. Notably,
   python does not seem to have a socket.AF_XDP, so I needed the C
   helper to make a socket and bind it to a queue to perform the test.

Tested this on my mlx5 machine and the test seems to pass.

Happy to take any suggestions / feedback on this one; sorry in advance
if I missed many obvious better ways to do things.

Thanks,
Joe

[1]: https://lore.kernel.org/netdev/20250113143109.60afa59a@kernel.org/

Joe Damato (2):
  netdev-genl: Add an XSK attribute to queues
  selftests: drv-net: Test queue xsk attribute

 Documentation/netlink/specs/netdev.yaml       | 10 ++-
 include/uapi/linux/netdev.h                   |  1 +
 net/core/netdev-genl.c                        |  6 ++
 tools/include/uapi/linux/netdev.h             |  1 +
 tools/testing/selftests/drivers/.gitignore    |  1 +
 tools/testing/selftests/drivers/net/Makefile  |  3 +
 tools/testing/selftests/drivers/net/queues.py | 32 ++++++-
 .../selftests/drivers/net/xdp_helper.c        | 90 +++++++++++++++++++
 8 files changed, 141 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/xdp_helper.c


base-commit: 0ad9617c78acbc71373fb341a6f75d4012b01d69
-- 
2.25.1

[RFC net-next 1/2] netdev-genl: Add an XSK attribute to queues

[Joe Damato]

Expose a new per-queue attribute, xsk, which indicates that a queue is
being used for AF_XDP. Update the documentation to more explicitly state
which queue types are linked.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 Documentation/netlink/specs/netdev.yaml | 10 +++++++++-
 include/uapi/linux/netdev.h             |  1 +
 net/core/netdev-genl.c                  |  6 ++++++
 tools/include/uapi/linux/netdev.h       |  1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index cbb544bd6c84..7a72788cce03 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -286,6 +286,8 @@ attribute-sets:
       -
         name: type
         doc: Queue type as rx, tx. Each queue type defines a separate ID space.
+             XDP TX queues allocated in the kernel are not linked to NAPIs and
+             thus not listed. AF_XDP queues have the xsk attribute set.
         type: u32
         enum: queue-type
       -
@@ -296,7 +298,12 @@ attribute-sets:
         name: dmabuf
         doc: ID of the dmabuf attached to this queue, if any.
         type: u32
-
+      -
+        name: xsk
+        doc: Non-zero for queues which are used for XSK (AF_XDP), 0 otherwise.
+        type: u32
+        checks:
+          max: 1
   -
     name: qstats
     doc: |
@@ -637,6 +644,7 @@ operations:
             - napi-id
             - ifindex
             - dmabuf
+            - xsk
       dump:
         request:
           attributes:
diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
index e4be227d3ad6..4d2dcf4960ec 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -136,6 +136,7 @@ enum {
 	NETDEV_A_QUEUE_TYPE,
 	NETDEV_A_QUEUE_NAPI_ID,
 	NETDEV_A_QUEUE_DMABUF,
+	NETDEV_A_QUEUE_XSK,
 
 	__NETDEV_A_QUEUE_MAX,
 	NETDEV_A_QUEUE_MAX = (__NETDEV_A_QUEUE_MAX - 1)
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 715f85c6b62e..964aebfcb079 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -394,12 +394,18 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
 		    nla_put_u32(rsp, NETDEV_A_QUEUE_DMABUF, binding->id))
 			goto nla_put_failure;
 
+		if (nla_put_u32(rsp, NETDEV_A_QUEUE_XSK, !!rxq->pool))
+			goto nla_put_failure;
+
 		break;
 	case NETDEV_QUEUE_TYPE_TX:
 		txq = netdev_get_tx_queue(netdev, q_idx);
 		if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID,
 					     txq->napi->napi_id))
 			goto nla_put_failure;
+
+		if (nla_put_u32(rsp, NETDEV_A_QUEUE_XSK, !!txq->pool))
+			goto nla_put_failure;
 	}
 
 	genlmsg_end(rsp, hdr);
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index e4be227d3ad6..4d2dcf4960ec 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -136,6 +136,7 @@ enum {
 	NETDEV_A_QUEUE_TYPE,
 	NETDEV_A_QUEUE_NAPI_ID,
 	NETDEV_A_QUEUE_DMABUF,
+	NETDEV_A_QUEUE_XSK,
 
 	__NETDEV_A_QUEUE_MAX,
 	NETDEV_A_QUEUE_MAX = (__NETDEV_A_QUEUE_MAX - 1)
-- 
2.25.1

[RFC net-next 2/2] selftests: drv-net: Test queue xsk attribute

[Joe Damato]

Test that queues which are used for AF_XDP have the xsk attribute set.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 tools/testing/selftests/drivers/.gitignore    |  1 +
 tools/testing/selftests/drivers/net/Makefile  |  3 +
 tools/testing/selftests/drivers/net/queues.py | 32 ++++++-
 .../selftests/drivers/net/xdp_helper.c        | 90 +++++++++++++++++++
 4 files changed, 124 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/xdp_helper.c

diff --git a/tools/testing/selftests/drivers/.gitignore b/tools/testing/selftests/drivers/.gitignore
index 09e23b5afa96..3c109144f7ff 100644
--- a/tools/testing/selftests/drivers/.gitignore
+++ b/tools/testing/selftests/drivers/.gitignore
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 /dma-buf/udmabuf
 /s390x/uvdevice/test_uvdevice
+/net/xdp_helper
diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
index 137470bdee0c..f6ec08680f48 100644
--- a/tools/testing/selftests/drivers/net/Makefile
+++ b/tools/testing/selftests/drivers/net/Makefile
@@ -1,10 +1,13 @@
 # SPDX-License-Identifier: GPL-2.0
+CFLAGS += $(KHDR_INCLUDES)
 
 TEST_INCLUDES := $(wildcard lib/py/*.py) \
 		 $(wildcard lib/sh/*.sh) \
 		 ../../net/net_helper.sh \
 		 ../../net/lib.sh \
 
+TEST_GEN_PROGS := xdp_helper
+
 TEST_PROGS := \
 	netcons_basic.sh \
 	netcons_overflow.sh \
diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index 38303da957ee..4bd4710b1a79 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -8,7 +8,10 @@ from lib.py import NetDrvEnv
 from lib.py import 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}-*')
@@ -21,6 +24,31 @@ def nl_get_queues(cfg, nl, qtype='rx'):
         return len([q for q in queues if q['type'] == qtype])
     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}"],
+                           stdin=subprocess.PIPE, stdout=subprocess.PIPE, bufsize=1,
+                           text=True)
+
+    stdout, stderr = xdp.communicate(timeout=10)
+    rx = tx = False
+
+    queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
+    if queues:
+        for q in queues:
+            if q['id'] == 0:
+                if q['type'] == 'rx':
+                    rx = True
+                if q['type'] == 'tx':
+                    tx = True
+
+                ksft_eq(q['xsk'], 1)
+            else:
+                ksft_eq(q['xsk'], 0)
+
+    ksft_eq(rx, True)
+    ksft_eq(tx, True)
+    xdp.kill()
 
 def get_queues(cfg, nl) -> None:
     snl = NetdevFamily(recv_size=4096)
@@ -81,7 +109,7 @@ def check_down(cfg, nl) -> None:
 
 def main() -> None:
     with NetDrvEnv(__file__, queue_count=100) as cfg:
-        ksft_run([get_queues, addremove_queues, check_down], args=(cfg, NetdevFamily()))
+        ksft_run([get_queues, addremove_queues, check_down, check_xdp], args=(cfg, NetdevFamily()))
     ksft_exit()
 
 
diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/drivers/net/xdp_helper.c
new file mode 100644
index 000000000000..8ed5f0e7233e
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/xdp_helper.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <sys/socket.h>
+#include <linux/if_xdp.h>
+#include <linux/if_link.h>
+#include <net/if.h>
+#include <inttypes.h>
+
+#define UMEM_SZ (1U << 16)
+#define NUM_DESC (UMEM_SZ / 2048)
+
+/**
+ * this is a simple helper program that creates an XDP socket and does the
+ * minimum necessary to get bind() to succeed.
+ *
+ * this test program is not intended to actually process packets, but could be
+ * extended in the future if that is actually needed.
+ *
+ * it is used by queues.py to ensure the xsk netlinux attribute is set
+ * correctly.
+ */
+int main(int argc, char **argv)
+{
+	struct xdp_umem_reg umem_reg = { 0 };
+	struct sockaddr_xdp sxdp = { 0 };
+	int num_desc = NUM_DESC;
+	void *umem_area;
+	int ifindex;
+	int sock_fd;
+	int queue;
+	char byte;
+
+	if (argc != 3) {
+		fprintf(stderr, "Usage: %s ifindex queue_id", argv[0]);
+		return 1;
+	}
+
+	sock_fd = socket(AF_XDP, SOCK_RAW, 0);
+	if (sock_fd < 0) {
+		perror("socket creation failed");
+		return 1;
+	}
+
+	ifindex = atoi(argv[1]);
+	queue = atoi(argv[2]);
+
+	umem_area = mmap(NULL, UMEM_SZ, PROT_READ | PROT_WRITE, MAP_PRIVATE |
+			MAP_ANONYMOUS, -1, 0);
+	if (umem_area == MAP_FAILED)
+		return -1;
+
+	umem_reg.addr = (uintptr_t)umem_area;
+	umem_reg.len = UMEM_SZ;
+	umem_reg.chunk_size = 2048;
+	umem_reg.headroom = 0;
+
+	setsockopt(sock_fd, SOL_XDP, XDP_UMEM_REG, &umem_reg,
+		   sizeof(umem_reg));
+	setsockopt(sock_fd, SOL_XDP, XDP_UMEM_FILL_RING, &num_desc,
+		   sizeof(num_desc));
+	setsockopt(sock_fd, SOL_XDP, XDP_UMEM_COMPLETION_RING, &num_desc,
+		   sizeof(num_desc));
+	setsockopt(sock_fd, SOL_XDP, XDP_RX_RING, &num_desc, sizeof(num_desc));
+
+	sxdp.sxdp_family = AF_XDP;
+	sxdp.sxdp_ifindex = ifindex;
+	sxdp.sxdp_queue_id = queue;
+	sxdp.sxdp_flags = 0;
+
+	if (bind(sock_fd, (struct sockaddr *)&sxdp, sizeof(sxdp)) != 0) {
+		perror("bind failed");
+		close(sock_fd);
+		return 1;
+	}
+
+	/* give the parent program some data when the socket is ready*/
+	fprintf(stdout, "%d\n", sock_fd);
+
+	/* parent program will write a byte to stdin when its ready for this
+	 * helper to exit
+	 */
+	read(STDIN_FILENO, &byte, 1);
+
+	close(sock_fd);
+	return 0;
+}
-- 
2.25.1

Re: [RFC net-next 0/2] netdevgenl: Add an xsk attribute to queues

[Joe Damato]

On Wed, Jan 29, 2025 at 05:24:23PM +0000, Joe Damato wrote:
> Greetings:
> 
> This is an attempt to followup on something Jakub asked me about [1],
> adding an xsk attribute to queues and more clearly documenting which
> queues are linked to NAPIs...
> 
> But:
> 
> 1. I couldn't pick a good "thing" to expose as "xsk", so I chose 0 or 1.
>    Happy to take suggestions on what might be better to expose for the
>    xsk queue attribute.
> 
> 2. I create a silly C helper program to create an XDP socket in order to
>    add a new test to queues.py. I'm not particularly good at python
>    programming, so there's probably a better way to do this. Notably,
>    python does not seem to have a socket.AF_XDP, so I needed the C
>    helper to make a socket and bind it to a queue to perform the test.
> 
> Tested this on my mlx5 machine and the test seems to pass.

I should have been slightly more specific, I ran queues.py two ways:

1. By setting NETIF= to my mlx5 NIC
2. By just running queues.py (without NETIF) set (which I presume
   uses netdevsim)

The test passes in both cases.

Re: [RFC net-next 1/2] netdev-genl: Add an XSK attribute to queues

[Jakub Kicinski]

On Wed, 29 Jan 2025 17:24:24 +0000 Joe Damato wrote:
> Expose a new per-queue attribute, xsk, which indicates that a queue is
> being used for AF_XDP. Update the documentation to more explicitly state
> which queue types are linked.

Let's do the same thing we did for io_uring queues? An empty nest:
https://lore.kernel.org/all/20250116231704.2402455-6-dw@davidwei.uk/

At the protocol level nest is both smaller and more flexible.
It's just 4B with zero length and a "this is a nest" flag.
We can add attributes to it as we think of things to express.

Re: [RFC net-next 2/2] selftests: drv-net: Test queue xsk attribute

[Jakub Kicinski]

On Wed, 29 Jan 2025 17:24:25 +0000 Joe Damato wrote:
> Test that queues which are used for AF_XDP have the xsk attribute set.

> diff --git a/tools/testing/selftests/drivers/.gitignore b/tools/testing/selftests/drivers/.gitignore
> index 09e23b5afa96..3c109144f7ff 100644
> --- a/tools/testing/selftests/drivers/.gitignore
> +++ b/tools/testing/selftests/drivers/.gitignore
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  /dma-buf/udmabuf
>  /s390x/uvdevice/test_uvdevice
> +/net/xdp_helper

Let's create our own gitignore, under drivers/net
we'll get conflicts with random trees if we add to the shared one

>  def sys_get_queues(ifname, qtype='rx') -> int:
>      folders = glob.glob(f'/sys/class/net/{ifname}/queues/{qtype}-*')
> @@ -21,6 +24,31 @@ def nl_get_queues(cfg, nl, qtype='rx'):
>          return len([q for q in queues if q['type'] == qtype])
>      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}"],
> +                           stdin=subprocess.PIPE, stdout=subprocess.PIPE, bufsize=1,
> +                           text=True)

add:
	defer(xdp.kill)

here, to make sure test cleanup will always try to kill the process,
then you can remove the xdp.kill() at the end

> +    stdout, stderr = xdp.communicate(timeout=10)
> +    rx = tx = False
> +
> +    queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
> +    if queues:

if not queues:
	raise KsftSkipEx("Netlink reports no queues")

That said only reason I can think of for no queues to be reported would
be that the device is down, which is very weird and we could as well
crash. So maybe the check for queues is not necessary ?

> +        for q in queues:
> +            if q['id'] == 0:
> +                if q['type'] == 'rx':
> +                    rx = True
> +                if q['type'] == 'tx':
> +                    tx = True
> +
> +                ksft_eq(q['xsk'], 1)
> +            else:
> +                ksft_eq(q['xsk'], 0)
> +
> +    ksft_eq(rx, True)
> +    ksft_eq(tx, True)
> +    xdp.kill()

Re: [RFC net-next 1/2] netdev-genl: Add an XSK attribute to queues

[Joe Damato]

On Wed, Jan 29, 2025 at 05:52:24PM -0800, Jakub Kicinski wrote:
> On Wed, 29 Jan 2025 17:24:24 +0000 Joe Damato wrote:
> > Expose a new per-queue attribute, xsk, which indicates that a queue is
> > being used for AF_XDP. Update the documentation to more explicitly state
> > which queue types are linked.
> 
> Let's do the same thing we did for io_uring queues? An empty nest:
> https://lore.kernel.org/all/20250116231704.2402455-6-dw@davidwei.uk/
> 
> At the protocol level nest is both smaller and more flexible.
> It's just 4B with zero length and a "this is a nest" flag.
> We can add attributes to it as we think of things to express.

Thanks for the pointer; will do.

Re: [RFC net-next 2/2] selftests: drv-net: Test queue xsk attribute

[Joe Damato]

On Wed, Jan 29, 2025 at 06:07:51PM -0800, Jakub Kicinski wrote:
> On Wed, 29 Jan 2025 17:24:25 +0000 Joe Damato wrote:
> > Test that queues which are used for AF_XDP have the xsk attribute set.
> 
> > diff --git a/tools/testing/selftests/drivers/.gitignore b/tools/testing/selftests/drivers/.gitignore
> > index 09e23b5afa96..3c109144f7ff 100644
> > --- a/tools/testing/selftests/drivers/.gitignore
> > +++ b/tools/testing/selftests/drivers/.gitignore
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  /dma-buf/udmabuf
> >  /s390x/uvdevice/test_uvdevice
> > +/net/xdp_helper
> 
> Let's create our own gitignore, under drivers/net
> we'll get conflicts with random trees if we add to the shared one

OK, SGTM.

> >  def sys_get_queues(ifname, qtype='rx') -> int:
> >      folders = glob.glob(f'/sys/class/net/{ifname}/queues/{qtype}-*')
> > @@ -21,6 +24,31 @@ def nl_get_queues(cfg, nl, qtype='rx'):
> >          return len([q for q in queues if q['type'] == qtype])
> >      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}"],
> > +                           stdin=subprocess.PIPE, stdout=subprocess.PIPE, bufsize=1,
> > +                           text=True)
> 
> add:
> 	defer(xdp.kill)
> 
> here, to make sure test cleanup will always try to kill the process,
> then you can remove the xdp.kill() at the end

OK, will do.

> > +    stdout, stderr = xdp.communicate(timeout=10)
> > +    rx = tx = False
> > +
> > +    queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
> > +    if queues:
> 
> if not queues:
> 	raise KsftSkipEx("Netlink reports no queues")
> 
> That said only reason I can think of for no queues to be reported would
> be that the device is down, which is very weird and we could as well
> crash. So maybe the check for queues is not necessary ?

I kind of feel like raising is slightly more verbose, which I tend
to slightly prefer over just a crash that might leave a future
person confused.

I'll go with the raise as you suggested instead.

Re: [RFC net-next 1/2] netdev-genl: Add an XSK attribute to queues

[Joe Damato]

On Wed, Jan 29, 2025 at 05:52:24PM -0800, Jakub Kicinski wrote:
> On Wed, 29 Jan 2025 17:24:24 +0000 Joe Damato wrote:
> > Expose a new per-queue attribute, xsk, which indicates that a queue is
> > being used for AF_XDP. Update the documentation to more explicitly state
> > which queue types are linked.
> 
> Let's do the same thing we did for io_uring queues? An empty nest:
> https://lore.kernel.org/all/20250116231704.2402455-6-dw@davidwei.uk/
> 
> At the protocol level nest is both smaller and more flexible.
> It's just 4B with zero length and a "this is a nest" flag.
> We can add attributes to it as we think of things to express.

I got a thing working locally, but just to make sure I'm
following... you are saying that the attribute will exist (but have
nothing in it) when the queue has a pool, and when !q->pool the
attribute will not exist?

For example:

[{'id': 0, 'ifindex': 5, 'napi-id': 8266, 'type': 'rx', 'xsk': {}},
 {'id': 1, 'ifindex': 5, 'napi-id': 8267, 'type': 'rx'},
 ...

Is that what you are thinking?

Completely fine with me as I haven't read enough of the xsk code to
really have a good sense of what attributes might be helpful to
expose at this point.

Re: [RFC net-next 1/2] netdev-genl: Add an XSK attribute to queues

[Jakub Kicinski]

On Thu, 30 Jan 2025 13:06:47 -0500 Joe Damato wrote:
> On Wed, Jan 29, 2025 at 05:52:24PM -0800, Jakub Kicinski wrote:
> > On Wed, 29 Jan 2025 17:24:24 +0000 Joe Damato wrote:  
> > > Expose a new per-queue attribute, xsk, which indicates that a queue is
> > > being used for AF_XDP. Update the documentation to more explicitly state
> > > which queue types are linked.  
> > 
> > Let's do the same thing we did for io_uring queues? An empty nest:
> > https://lore.kernel.org/all/20250116231704.2402455-6-dw@davidwei.uk/
> > 
> > At the protocol level nest is both smaller and more flexible.
> > It's just 4B with zero length and a "this is a nest" flag.
> > We can add attributes to it as we think of things to express.  
> 
> I got a thing working locally, but just to make sure I'm
> following... you are saying that the attribute will exist (but have
> nothing in it) when the queue has a pool, and when !q->pool the
> attribute will not exist?
> 
> For example:
> 
> [{'id': 0, 'ifindex': 5, 'napi-id': 8266, 'type': 'rx', 'xsk': {}},
>  {'id': 1, 'ifindex': 5, 'napi-id': 8267, 'type': 'rx'},
>  ...
> 
> Is that what you are thinking?

Yup! That's it.

> Completely fine with me as I haven't read enough of the xsk code to
> really have a good sense of what attributes might be helpful to
> expose at this point.