netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v3 0/2] netdevgenl: Add an xsk attribute to queues
@ 2025-02-04 19:10 Joe Damato
  2025-02-04 19:10 ` [PATCH net-next v3 1/2] netdev-genl: Add an XSK " Joe Damato
  2025-02-04 19:10 ` [PATCH net-next v3 2/2] selftests: drv-net: Test queue xsk attribute Joe Damato
  0 siblings, 2 replies; 8+ messages in thread
From: Joe Damato @ 2025-02-04 19:10 UTC (permalink / raw)
  To: netdev
  Cc: kuba, Joe Damato, Alexei Starovoitov, Andrew Lunn,
	open list:XDP (eXpress Data Path):Keyword:(?:b|_)xdp(?:b|_),
	Daniel Borkmann, Daniel Jurgens, David S. Miller, Donald Hunter,
	Eric Dumazet, Jesper Dangaard Brouer, John Fastabend, open list,
	open list:KERNEL SELFTEST FRAMEWORK, Martin Karsten, Mina Almasry,
	Paolo Abeni, Shuah Khan, Simon Horman, Stanislav Fomichev,
	Xuan Zhuo

Greetings:

Welcome to v3. No functional changes, see changelog below.

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

After the RFC [2], Jakub suggested creating an empty nest for queues
which have a pool, so I've adjusted this version to work that way.

The nest can be extended in the future to express attributes about XSK
as needed. Queues which are not used for AF_XDP do not have the xsk
attribute present.

I've run the included test on:
  - my mlx5 machine (via NETIF=)
  - without setting NETIF

And the test seems to pass in both cases.

Thanks,
Joe

[1]: https://lore.kernel.org/netdev/20250113143109.60afa59a@kernel.org/
[2]: https://lore.kernel.org/netdev/20250129172431.65773-1-jdamato@fastly.com/

v3:
  - Change comment format in patch 2 to avoid kdoc warnings. No other
    changes.

v2: https://lore.kernel.org/all/20250203185828.19334-1-jdamato@fastly.com/
  - Switched from RFC to actual submission now that net-next is open
  - Adjusted patch 1 to include an empty nest as suggested by Jakub
  - Adjusted patch 2 to update the test based on changes to patch 1, and
    to incorporate some Python feedback from Jakub :)

rfc: https://lore.kernel.org/netdev/20250129172431.65773-1-jdamato@fastly.com/

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

 Documentation/netlink/specs/netdev.yaml       | 13 ++-
 include/uapi/linux/netdev.h                   |  6 ++
 net/core/netdev-genl.c                        | 11 +++
 tools/include/uapi/linux/netdev.h             |  6 ++
 .../testing/selftests/drivers/net/.gitignore  |  2 +
 tools/testing/selftests/drivers/net/Makefile  |  3 +
 tools/testing/selftests/drivers/net/queues.py | 35 +++++++-
 .../selftests/drivers/net/xdp_helper.c        | 89 +++++++++++++++++++
 8 files changed, 162 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/.gitignore
 create mode 100644 tools/testing/selftests/drivers/net/xdp_helper.c


base-commit: c2933b2befe25309f4c5cfbea0ca80909735fd76
-- 
2.43.0


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

* [PATCH net-next v3 1/2] netdev-genl: Add an XSK attribute to queues
  2025-02-04 19:10 [PATCH net-next v3 0/2] netdevgenl: Add an xsk attribute to queues Joe Damato
@ 2025-02-04 19:10 ` Joe Damato
  2025-02-07  0:57   ` Jakub Kicinski
  2025-02-07  6:43   ` kernel test robot
  2025-02-04 19:10 ` [PATCH net-next v3 2/2] selftests: drv-net: Test queue xsk attribute Joe Damato
  1 sibling, 2 replies; 8+ messages in thread
From: Joe Damato @ 2025-02-04 19:10 UTC (permalink / raw)
  To: netdev
  Cc: kuba, Joe Damato, Donald Hunter, David S. Miller, Eric Dumazet,
	Paolo Abeni, Simon Horman, Andrew Lunn, Xuan Zhuo,
	Stanislav Fomichev, Mina Almasry, Daniel Jurgens, Martin Karsten,
	open list

Expose a new per-queue nest attribute, xsk, which will be present for
queues that are being used for AF_XDP. If the queue is not being used for
AF_XDP, the nest will not be present.

In the future, this attribute can be extended to include more data about
XSK as it is needed.

Signed-off-by: Joe Damato <jdamato@fastly.com>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
---
 v2:
   - Patch adjusted to include an attribute, xsk, which is an empty nest
     and exposed for queues which have a pool.

 Documentation/netlink/specs/netdev.yaml | 13 ++++++++++++-
 include/uapi/linux/netdev.h             |  6 ++++++
 net/core/netdev-genl.c                  | 11 +++++++++++
 tools/include/uapi/linux/netdev.h       |  6 ++++++
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index cbb544bd6c84..4c3eda5ba754 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -268,6 +268,9 @@ attribute-sets:
         doc: The timeout, in nanoseconds, of how long to suspend irq
              processing, if event polling finds events
         type: uint
+  -
+    name: xsk-info
+    attributes: []
   -
     name: queue
     attributes:
@@ -286,6 +289,9 @@ 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 will have more information set in
+             the xsk attribute.
         type: u32
         enum: queue-type
       -
@@ -296,7 +302,11 @@ attribute-sets:
         name: dmabuf
         doc: ID of the dmabuf attached to this queue, if any.
         type: u32
-
+      -
+        name: xsk
+        doc: XSK information for this queue, if any.
+        type: nest
+        nested-attributes: xsk-info
   -
     name: qstats
     doc: |
@@ -637,6 +647,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..46bdb0b67a39 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -130,12 +130,18 @@ enum {
 	NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
 };
 
+enum {
+	__NETDEV_A_XSK_INFO_MAX,
+	NETDEV_A_XSK_INFO_MAX = (__NETDEV_A_XSK_INFO_MAX - 1)
+};
+
 enum {
 	NETDEV_A_QUEUE_ID = 1,
 	NETDEV_A_QUEUE_IFINDEX,
 	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..efaccfb6438e 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -371,6 +371,7 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
 	struct net_devmem_dmabuf_binding *binding;
 	struct netdev_rx_queue *rxq;
 	struct netdev_queue *txq;
+	struct nlattr *nest;
 	void *hdr;
 
 	hdr = genlmsg_iput(rsp, info);
@@ -394,12 +395,22 @@ 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 (rxq->pool) {
+			nest = nla_nest_start(rsp, NETDEV_A_QUEUE_XSK);
+			nla_nest_end(rsp, nest);
+		}
+
 		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 (txq->pool) {
+			nest = nla_nest_start(rsp, NETDEV_A_QUEUE_XSK);
+			nla_nest_end(rsp, nest);
+		}
 	}
 
 	genlmsg_end(rsp, hdr);
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index e4be227d3ad6..46bdb0b67a39 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -130,12 +130,18 @@ enum {
 	NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
 };
 
+enum {
+	__NETDEV_A_XSK_INFO_MAX,
+	NETDEV_A_XSK_INFO_MAX = (__NETDEV_A_XSK_INFO_MAX - 1)
+};
+
 enum {
 	NETDEV_A_QUEUE_ID = 1,
 	NETDEV_A_QUEUE_IFINDEX,
 	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.43.0


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

* [PATCH net-next v3 2/2] selftests: drv-net: Test queue xsk attribute
  2025-02-04 19:10 [PATCH net-next v3 0/2] netdevgenl: Add an xsk attribute to queues Joe Damato
  2025-02-04 19:10 ` [PATCH net-next v3 1/2] netdev-genl: Add an XSK " Joe Damato
@ 2025-02-04 19:10 ` Joe Damato
  1 sibling, 0 replies; 8+ messages in thread
From: Joe Damato @ 2025-02-04 19:10 UTC (permalink / raw)
  To: netdev
  Cc: kuba, Joe Damato, Andrew Lunn, David S. Miller, Eric Dumazet,
	Paolo Abeni, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, open list,
	open list:KERNEL SELFTEST FRAMEWORK,
	open list:XDP (eXpress Data Path):Keyword:(?:b|_)xdp(?:b|_)

Test that queues which are used for AF_XDP have the xsk nest attribute.
The attribute is currently empty, but its existence means the AF_XDP is
being used for the queue.

Signed-off-by: Joe Damato <jdamato@fastly.com>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
---
 v3:
   - Change comment style of helper C program to avoid kdoc warnings as
     suggested by Jakub. No other changes.

 v2:
   - Updated the Python test after changes to patch 1 which expose an
     empty nest
   - Updated Python test with general Python coding feedback

 .../testing/selftests/drivers/net/.gitignore  |  2 +
 tools/testing/selftests/drivers/net/Makefile  |  3 +
 tools/testing/selftests/drivers/net/queues.py | 35 +++++++-
 .../selftests/drivers/net/xdp_helper.c        | 89 +++++++++++++++++++
 4 files changed, 127 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/.gitignore
 create mode 100644 tools/testing/selftests/drivers/net/xdp_helper.c

diff --git a/tools/testing/selftests/drivers/net/.gitignore b/tools/testing/selftests/drivers/net/.gitignore
new file mode 100644
index 000000000000..ec746f374e85
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+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..55c2b296ad3c 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,34 @@ 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)
+    defer(xdp.kill)
+
+    stdout, stderr = xdp.communicate(timeout=10)
+    rx = tx = False
+
+    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
+
+            ksft_eq(q['xsk'], {})
+        else:
+            if 'xsk' in q:
+                _fail("Check failed: xsk attribute set.")
+
+    ksft_eq(rx, True)
+    ksft_eq(tx, True)
 
 def get_queues(cfg, nl) -> None:
     snl = NetdevFamily(recv_size=4096)
@@ -81,7 +112,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..b04d4e0ea30a
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/xdp_helper.c
@@ -0,0 +1,89 @@
+// 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.43.0


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

* Re: [PATCH net-next v3 1/2] netdev-genl: Add an XSK attribute to queues
  2025-02-04 19:10 ` [PATCH net-next v3 1/2] netdev-genl: Add an XSK " Joe Damato
@ 2025-02-07  0:57   ` Jakub Kicinski
  2025-02-07  1:31     ` Joe Damato
  2025-02-07  6:43   ` kernel test robot
  1 sibling, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2025-02-07  0:57 UTC (permalink / raw)
  To: Joe Damato
  Cc: netdev, Donald Hunter, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Andrew Lunn, Xuan Zhuo, Stanislav Fomichev,
	Mina Almasry, Daniel Jurgens, Martin Karsten, open list

On Tue,  4 Feb 2025 19:10:47 +0000 Joe Damato wrote:
> +		if (rxq->pool) {
> +			nest = nla_nest_start(rsp, NETDEV_A_QUEUE_XSK);
> +			nla_nest_end(rsp, nest);
> +		}

nla_nest_start() can fail, you gotta nul-check the return value.
You could possibly add an nla_put_empty_nest() helper in netlink.h
to make this less awkward? I think the iouring guys had the same bug

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

* Re: [PATCH net-next v3 1/2] netdev-genl: Add an XSK attribute to queues
  2025-02-07  0:57   ` Jakub Kicinski
@ 2025-02-07  1:31     ` Joe Damato
  2025-02-07  1:41       ` Jakub Kicinski
  0 siblings, 1 reply; 8+ messages in thread
From: Joe Damato @ 2025-02-07  1:31 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, Donald Hunter, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Andrew Lunn, Xuan Zhuo, Stanislav Fomichev,
	Mina Almasry, Daniel Jurgens, Martin Karsten, open list

On Thu, Feb 06, 2025 at 04:57:46PM -0800, Jakub Kicinski wrote:
> On Tue,  4 Feb 2025 19:10:47 +0000 Joe Damato wrote:
> > +		if (rxq->pool) {
> > +			nest = nla_nest_start(rsp, NETDEV_A_QUEUE_XSK);
> > +			nla_nest_end(rsp, nest);
> > +		}
> 
> nla_nest_start() can fail, you gotta nul-check the return value.
> You could possibly add an nla_put_empty_nest() helper in netlink.h
> to make this less awkward? I think the iouring guys had the same bug

Ah, right.

I'll see what a helper looks like. Feels like maybe overkill?

Thanks for the review.

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

* Re: [PATCH net-next v3 1/2] netdev-genl: Add an XSK attribute to queues
  2025-02-07  1:31     ` Joe Damato
@ 2025-02-07  1:41       ` Jakub Kicinski
  2025-02-07  1:46         ` Joe Damato
  0 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2025-02-07  1:41 UTC (permalink / raw)
  To: Joe Damato
  Cc: netdev, Donald Hunter, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Andrew Lunn, Xuan Zhuo, Stanislav Fomichev,
	Mina Almasry, Daniel Jurgens, Martin Karsten, open list

On Thu, 6 Feb 2025 17:31:47 -0800 Joe Damato wrote:
> > nla_nest_start() can fail, you gotta nul-check the return value.
> > You could possibly add an nla_put_empty_nest() helper in netlink.h
> > to make this less awkward? I think the iouring guys had the same bug  
> 
> Ah, right.
> 
> I'll see what a helper looks like. Feels like maybe overkill?

Yeah, not sure either. Technically nla_nest_end() isn't required here,
but that's not very obvious to a casual reader. So a helper that hides
that fact could be useful:

static inline int nla_put_empty_nest(struct sk_buff *skb, int attrtype)
{
	return nla_nest_start(skb, attrtype) ? 0 : -EMSGSIZE;
}

But totally unsure whether it's worthwhile. Just don't want for someone
to suggest this on v4 and make you respin once again.

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

* Re: [PATCH net-next v3 1/2] netdev-genl: Add an XSK attribute to queues
  2025-02-07  1:41       ` Jakub Kicinski
@ 2025-02-07  1:46         ` Joe Damato
  0 siblings, 0 replies; 8+ messages in thread
From: Joe Damato @ 2025-02-07  1:46 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, Donald Hunter, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Andrew Lunn, Xuan Zhuo, Stanislav Fomichev,
	Mina Almasry, Daniel Jurgens, Martin Karsten, open list

On Thu, Feb 06, 2025 at 05:41:38PM -0800, Jakub Kicinski wrote:
> On Thu, 6 Feb 2025 17:31:47 -0800 Joe Damato wrote:
> > > nla_nest_start() can fail, you gotta nul-check the return value.
> > > You could possibly add an nla_put_empty_nest() helper in netlink.h
> > > to make this less awkward? I think the iouring guys had the same bug  
> > 
> > Ah, right.
> > 
> > I'll see what a helper looks like. Feels like maybe overkill?
> 
> Yeah, not sure either. Technically nla_nest_end() isn't required here,
> but that's not very obvious to a casual reader. So a helper that hides
> that fact could be useful:
> 
> static inline int nla_put_empty_nest(struct sk_buff *skb, int attrtype)
> {
> 	return nla_nest_start(skb, attrtype) ? 0 : -EMSGSIZE;
> }

Yea after reading the code it makes sense that nla_nest_end is not
needed. I wrote a small thing similar to what you proposed above,
but yours is more succinct.

I'll go with that and see how it looks.

> But totally unsure whether it's worthwhile. Just don't want for someone
> to suggest this on v4 and make you respin once again.

No worries; respinning is not the end of the world.

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

* Re: [PATCH net-next v3 1/2] netdev-genl: Add an XSK attribute to queues
  2025-02-04 19:10 ` [PATCH net-next v3 1/2] netdev-genl: Add an XSK " Joe Damato
  2025-02-07  0:57   ` Jakub Kicinski
@ 2025-02-07  6:43   ` kernel test robot
  1 sibling, 0 replies; 8+ messages in thread
From: kernel test robot @ 2025-02-07  6:43 UTC (permalink / raw)
  To: Joe Damato, netdev
  Cc: llvm, oe-kbuild-all, kuba, Joe Damato, Donald Hunter,
	Eric Dumazet, Paolo Abeni, Simon Horman, Andrew Lunn, Xuan Zhuo,
	Stanislav Fomichev, Mina Almasry, Daniel Jurgens, Martin Karsten,
	linux-kernel

Hi Joe,

kernel test robot noticed the following build errors:

[auto build test ERROR on c2933b2befe25309f4c5cfbea0ca80909735fd76]

url:    https://github.com/intel-lab-lkp/linux/commits/Joe-Damato/netdev-genl-Add-an-XSK-attribute-to-queues/20250205-031236
base:   c2933b2befe25309f4c5cfbea0ca80909735fd76
patch link:    https://lore.kernel.org/r/20250204191108.161046-2-jdamato%40fastly.com
patch subject: [PATCH net-next v3 1/2] netdev-genl: Add an XSK attribute to queues
config: arm-defconfig (https://download.01.org/0day-ci/archive/20250207/202502071452.B85Lw7aV-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project ee3bccab34f57387bdf33853cdd5f214fef349a2)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250207/202502071452.B85Lw7aV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502071452.B85Lw7aV-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/core/netdev-genl.c:398:12: error: no member named 'pool' in 'struct netdev_rx_queue'
     398 |                 if (rxq->pool) {
         |                     ~~~  ^
>> net/core/netdev-genl.c:410:12: error: no member named 'pool' in 'struct netdev_queue'
     410 |                 if (txq->pool) {
         |                     ~~~  ^
   2 errors generated.


vim +398 net/core/netdev-genl.c

   366	
   367	static int
   368	netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
   369				 u32 q_idx, u32 q_type, const struct genl_info *info)
   370	{
   371		struct net_devmem_dmabuf_binding *binding;
   372		struct netdev_rx_queue *rxq;
   373		struct netdev_queue *txq;
   374		struct nlattr *nest;
   375		void *hdr;
   376	
   377		hdr = genlmsg_iput(rsp, info);
   378		if (!hdr)
   379			return -EMSGSIZE;
   380	
   381		if (nla_put_u32(rsp, NETDEV_A_QUEUE_ID, q_idx) ||
   382		    nla_put_u32(rsp, NETDEV_A_QUEUE_TYPE, q_type) ||
   383		    nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex))
   384			goto nla_put_failure;
   385	
   386		switch (q_type) {
   387		case NETDEV_QUEUE_TYPE_RX:
   388			rxq = __netif_get_rx_queue(netdev, q_idx);
   389			if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID,
   390						     rxq->napi->napi_id))
   391				goto nla_put_failure;
   392	
   393			binding = rxq->mp_params.mp_priv;
   394			if (binding &&
   395			    nla_put_u32(rsp, NETDEV_A_QUEUE_DMABUF, binding->id))
   396				goto nla_put_failure;
   397	
 > 398			if (rxq->pool) {
   399				nest = nla_nest_start(rsp, NETDEV_A_QUEUE_XSK);
   400				nla_nest_end(rsp, nest);
   401			}
   402	
   403			break;
   404		case NETDEV_QUEUE_TYPE_TX:
   405			txq = netdev_get_tx_queue(netdev, q_idx);
   406			if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID,
   407						     txq->napi->napi_id))
   408				goto nla_put_failure;
   409	
 > 410			if (txq->pool) {
   411				nest = nla_nest_start(rsp, NETDEV_A_QUEUE_XSK);
   412				nla_nest_end(rsp, nest);
   413			}
   414		}
   415	
   416		genlmsg_end(rsp, hdr);
   417	
   418		return 0;
   419	
   420	nla_put_failure:
   421		genlmsg_cancel(rsp, hdr);
   422		return -EMSGSIZE;
   423	}
   424	

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

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

end of thread, other threads:[~2025-02-07  6:43 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-04 19:10 [PATCH net-next v3 0/2] netdevgenl: Add an xsk attribute to queues Joe Damato
2025-02-04 19:10 ` [PATCH net-next v3 1/2] netdev-genl: Add an XSK " Joe Damato
2025-02-07  0:57   ` Jakub Kicinski
2025-02-07  1:31     ` Joe Damato
2025-02-07  1:41       ` Jakub Kicinski
2025-02-07  1:46         ` Joe Damato
2025-02-07  6:43   ` kernel test robot
2025-02-04 19:10 ` [PATCH net-next v3 2/2] selftests: drv-net: Test queue xsk attribute Joe Damato

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