Thread (58 messages) 58 messages, 4 authors, 2025-08-25

Re: [PATCH net-next v3 18/23] netdev: add support for setting rx-buf-len per queue

From: Mina Almasry <hidden>
Date: 2025-08-19 22:36:18
Also in: io-uring, lkml

On Mon, Aug 18, 2025 at 6:57 AM Pavel Begunkov [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Jakub Kicinski <kuba@kernel.org>

Zero-copy APIs increase the cost of buffer management. They also extend
this cost to user space applications which may be used to dealing with
much larger buffers. Allow setting rx-buf-len per queue, devices with
HW-GRO support can commonly fill buffers up to 32k (or rather 64k - 1
but that's not a power of 2..)

The implementation adds a new option to the netdev netlink, rather
than ethtool. The NIC-wide setting lives in ethtool ringparams so
one could argue that we should be extending the ethtool API.
OTOH netdev API is where we already have queue-get, and it's how
zero-copy applications bind memory providers.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 Documentation/netlink/specs/netdev.yaml | 15 ++++
 include/net/netdev_queues.h             |  5 ++
 include/net/netlink.h                   | 19 +++++
 include/uapi/linux/netdev.h             |  2 +
 net/core/netdev-genl-gen.c              | 15 ++++
 net/core/netdev-genl-gen.h              |  1 +
 net/core/netdev-genl.c                  | 92 +++++++++++++++++++++++++
 net/core/netdev_config.c                | 16 +++++
 tools/include/uapi/linux/netdev.h       |  2 +
 9 files changed, 167 insertions(+)
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index c035dc0f64fd..498c4bcafdbd 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -338,6 +338,10 @@ attribute-sets:
         doc: XSK information for this queue, if any.
         type: nest
         nested-attributes: xsk-info
+      -
+        name: rx-buf-len
+        doc: Per-queue configuration of ETHTOOL_A_RINGS_RX_BUF_LEN.
+        type: u32
   -
     name: qstats
     doc: |
@@ -771,6 +775,17 @@ operations:
         reply:
           attributes:
             - id
+    -
+      name: queue-set
+      doc: Set per-queue configurable options.
+      attribute-set: queue
+      do:
+        request:
+          attributes:
+            - ifindex
+            - type
+            - id
+            - rx-buf-len

 kernel-family:
   headers: ["net/netdev_netlink.h"]
diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
index d0cc475ec51e..b69b1d519dcb 100644
--- a/include/net/netdev_queues.h
+++ b/include/net/netdev_queues.h
@@ -39,6 +39,7 @@ struct netdev_config {

 /* Same semantics as fields in struct netdev_config */
 struct netdev_queue_config {
+       u32     rx_buf_len;
 };

 /* See the netdev.yaml spec for definition of each statistic */
@@ -141,6 +142,8 @@ void netdev_stat_queue_sum(struct net_device *netdev,
 /**
  * struct netdev_queue_mgmt_ops - netdev ops for queue management
  *
+ * @supported_ring_params: ring params supported per queue (ETHTOOL_RING_USE_*).
+ *
Not necessarily a problem, but note that if you depend on
ETHTOOL_RING_USE_*, then queue configs need to also be ethtool
configs, which means they need to also be NIC wide configs. Maybe
that's a plus in your eyes. I wonder if ever we're going to be in a
situation where some config makes sense per queue, but not per NIC.
mem providers are already that sorta.
quoted hunk ↗ jump to hunk
  * @ndo_queue_mem_size: Size of the struct that describes a queue's memory.
  *
  * @ndo_queue_cfg_defaults: (Optional) Populate queue config struct with
@@ -171,6 +174,8 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  * be called for an interface which is open.
  */
 struct netdev_queue_mgmt_ops {
+       u32     supported_ring_params;
+
        size_t  ndo_queue_mem_size;
        void    (*ndo_queue_cfg_defaults)(struct net_device *dev,
                                          int idx,
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 1a8356ca4b78..29989ad81ddd 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -2200,6 +2200,25 @@ static inline struct nla_bitfield32 nla_get_bitfield32(const struct nlattr *nla)
        return tmp;
 }

+/**
+ * nla_update_u32() - update u32 value from NLA_U32 attribute
+ * @dst:  value to update
+ * @attr: netlink attribute with new value or null
+ *
+ * Copy the u32 value from NLA_U32 netlink attribute @attr into variable
+ * pointed to by @dst; do nothing if @attr is null.
+ *
+ * Return: true if this function changed the value of @dst, otherwise false.
+ */
+static inline bool nla_update_u32(u32 *dst, const struct nlattr *attr)
+{
+       u32 old_val = *dst;
+
+       if (attr)
+               *dst = nla_get_u32(attr);
+       return *dst != old_val;
+}
+
 /**
  * nla_memdup - duplicate attribute memory (kmemdup)
  * @src: netlink attribute to duplicate from
diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
index 48eb49aa03d4..820f89b67a72 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -158,6 +158,7 @@ enum {
        NETDEV_A_QUEUE_DMABUF,
        NETDEV_A_QUEUE_IO_URING,
        NETDEV_A_QUEUE_XSK,
+       NETDEV_A_QUEUE_RX_BUF_LEN,

        __NETDEV_A_QUEUE_MAX,
        NETDEV_A_QUEUE_MAX = (__NETDEV_A_QUEUE_MAX - 1)
@@ -226,6 +227,7 @@ enum {
        NETDEV_CMD_BIND_RX,
        NETDEV_CMD_NAPI_SET,
        NETDEV_CMD_BIND_TX,
+       NETDEV_CMD_QUEUE_SET,

        __NETDEV_CMD_MAX,
        NETDEV_CMD_MAX = (__NETDEV_CMD_MAX - 1)
diff --git a/net/core/netdev-genl-gen.c b/net/core/netdev-genl-gen.c
index e9a2a6f26cb7..d053306a3af8 100644
--- a/net/core/netdev-genl-gen.c
+++ b/net/core/netdev-genl-gen.c
@@ -106,6 +106,14 @@ static const struct nla_policy netdev_bind_tx_nl_policy[NETDEV_A_DMABUF_FD + 1]
        [NETDEV_A_DMABUF_FD] = { .type = NLA_U32, },
 };

+/* NETDEV_CMD_QUEUE_SET - do */
+static const struct nla_policy netdev_queue_set_nl_policy[NETDEV_A_QUEUE_RX_BUF_LEN + 1] = {
+       [NETDEV_A_QUEUE_IFINDEX] = NLA_POLICY_MIN(NLA_U32, 1),
+       [NETDEV_A_QUEUE_TYPE] = NLA_POLICY_MAX(NLA_U32, 1),
+       [NETDEV_A_QUEUE_ID] = { .type = NLA_U32, },
+       [NETDEV_A_QUEUE_RX_BUF_LEN] = { .type = NLA_U32, },
+};
+
 /* Ops table for netdev */
 static const struct genl_split_ops netdev_nl_ops[] = {
        {
@@ -204,6 +212,13 @@ static const struct genl_split_ops netdev_nl_ops[] = {
                .maxattr        = NETDEV_A_DMABUF_FD,
                .flags          = GENL_CMD_CAP_DO,
        },
+       {
+               .cmd            = NETDEV_CMD_QUEUE_SET,
+               .doit           = netdev_nl_queue_set_doit,
+               .policy         = netdev_queue_set_nl_policy,
+               .maxattr        = NETDEV_A_QUEUE_RX_BUF_LEN,
+               .flags          = GENL_CMD_CAP_DO,
+       },
 };

 static const struct genl_multicast_group netdev_nl_mcgrps[] = {
diff --git a/net/core/netdev-genl-gen.h b/net/core/netdev-genl-gen.h
index cf3fad74511f..b7f5e5d9fca9 100644
--- a/net/core/netdev-genl-gen.h
+++ b/net/core/netdev-genl-gen.h
@@ -35,6 +35,7 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
 int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info);
 int netdev_nl_napi_set_doit(struct sk_buff *skb, struct genl_info *info);
 int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info);
+int netdev_nl_queue_set_doit(struct sk_buff *skb, struct genl_info *info);

 enum {
        NETDEV_NLGRP_MGMT,
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 6314eb7bdf69..abb128e45fcf 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -386,6 +386,30 @@ static int nla_put_napi_id(struct sk_buff *skb, const struct napi_struct *napi)
        return 0;
 }

+static int
+netdev_nl_queue_fill_cfg(struct sk_buff *rsp, struct net_device *netdev,
+                        u32 q_idx, u32 q_type)
+{
+       struct netdev_queue_config *qcfg;
+
+       if (!netdev_need_ops_lock(netdev))
+               return 0;
+
Why are we checking this? I don't see this function doing any locking.
quoted hunk ↗ jump to hunk
+       qcfg = &netdev->cfg->qcfg[q_idx];
+       switch (q_type) {
+       case NETDEV_QUEUE_TYPE_RX:
+               if (qcfg->rx_buf_len &&
+                   nla_put_u32(rsp, NETDEV_A_QUEUE_RX_BUF_LEN,
+                               qcfg->rx_buf_len))
+                       return -EMSGSIZE;
+               break;
+       default:
+               break;
+       }
+
+       return 0;
+}
+
 static int
 netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
                         u32 q_idx, u32 q_type, const struct genl_info *info)
@@ -433,6 +457,9 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
                break;
        }

+       if (netdev_nl_queue_fill_cfg(rsp, netdev, q_idx, q_type))
+               goto nla_put_failure;
+
        genlmsg_end(rsp, hdr);

        return 0;
@@ -572,6 +599,71 @@ int netdev_nl_queue_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
        return err;
 }

+int netdev_nl_queue_set_doit(struct sk_buff *skb, struct genl_info *info)
+{
+       struct nlattr * const *tb = info->attrs;
+       struct netdev_queue_config *qcfg;
+       u32 q_id, q_type, ifindex;
+       struct net_device *netdev;
+       bool mod;
+       int ret;
+
+       if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) ||
+           GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) ||
+           GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX))
+               return -EINVAL;
+
+       q_id = nla_get_u32(tb[NETDEV_A_QUEUE_ID]);
+       q_type = nla_get_u32(tb[NETDEV_A_QUEUE_TYPE]);
+       ifindex = nla_get_u32(tb[NETDEV_A_QUEUE_IFINDEX]);
+
+       if (q_type != NETDEV_QUEUE_TYPE_RX) {
+               /* Only Rx params exist right now */
+               NL_SET_BAD_ATTR(info->extack, tb[NETDEV_A_QUEUE_TYPE]);
+               return -EINVAL;
+       }
+
+       ret = 0;
+       netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
+       if (!netdev || !netif_device_present(netdev))
+               ret = -ENODEV;
+       else if (!netdev->queue_mgmt_ops)
+               ret = -EOPNOTSUPP;
+       if (ret) {
+               NL_SET_BAD_ATTR(info->extack, tb[NETDEV_A_QUEUE_IFINDEX]);
+               goto exit_unlock;
+       }
+
+       ret = netdev_nl_queue_validate(netdev, q_id, q_type);
+       if (ret) {
+               NL_SET_BAD_ATTR(info->extack, tb[NETDEV_A_QUEUE_ID]);
+               goto exit_unlock;
+       }
+
+       ret = netdev_reconfig_start(netdev);
+       if (ret)
+               goto exit_unlock;
+
+       qcfg = &netdev->cfg_pending->qcfg[q_id];
+       mod = nla_update_u32(&qcfg->rx_buf_len, tb[NETDEV_A_QUEUE_RX_BUF_LEN]);
Don't you need to check the queue_mgmt_ops->supported_thingy first to
know that this driver actually supports modifying rx_buf_len?
quoted hunk ↗ jump to hunk
+       if (!mod)
+               goto exit_free_cfg;
+
+       ret = netdev_rx_queue_restart(netdev, q_id, info->extack);
+       if (ret)
+               goto exit_free_cfg;
+
+       swap(netdev->cfg, netdev->cfg_pending);
+
+exit_free_cfg:
+       __netdev_free_config(netdev->cfg_pending);
+       netdev->cfg_pending = netdev->cfg;
+exit_unlock:
+       if (netdev)
+               netdev_unlock(netdev);
+       return ret;
+}
+
 #define NETDEV_STAT_NOT_SET            (~0ULL)

 static void netdev_nl_stats_add(void *_sum, const void *_add, size_t size)
diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
index fc700b77e4eb..ede02b77470e 100644
--- a/net/core/netdev_config.c
+++ b/net/core/netdev_config.c
@@ -67,11 +67,27 @@ int netdev_reconfig_start(struct net_device *dev)
 void __netdev_queue_config(struct net_device *dev, int rxq,
                           struct netdev_queue_config *qcfg, bool pending)
 {
+       const struct netdev_config *cfg;
+
+       cfg = pending ? dev->cfg_pending : dev->cfg;
+
        memset(qcfg, 0, sizeof(*qcfg));

        /* Get defaults from the driver, in case user config not set */
        if (dev->queue_mgmt_ops->ndo_queue_cfg_defaults)
                dev->queue_mgmt_ops->ndo_queue_cfg_defaults(dev, rxq, qcfg);
+
+       /* Set config based on device-level settings */
+       if (cfg->rx_buf_len)
+               qcfg->rx_buf_len = cfg->rx_buf_len;
+
+       /* Set config dedicated to this queue */
+       if (rxq >= 0) {
+               const struct netdev_queue_config *user_cfg = &cfg->qcfg[rxq];
+
+               if (user_cfg->rx_buf_len)
+                       qcfg->rx_buf_len = user_cfg->rx_buf_len;
+       }
 }

 /**
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index 48eb49aa03d4..820f89b67a72 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -158,6 +158,7 @@ enum {
        NETDEV_A_QUEUE_DMABUF,
        NETDEV_A_QUEUE_IO_URING,
        NETDEV_A_QUEUE_XSK,
+       NETDEV_A_QUEUE_RX_BUF_LEN,

        __NETDEV_A_QUEUE_MAX,
        NETDEV_A_QUEUE_MAX = (__NETDEV_A_QUEUE_MAX - 1)
@@ -226,6 +227,7 @@ enum {
        NETDEV_CMD_BIND_RX,
        NETDEV_CMD_NAPI_SET,
        NETDEV_CMD_BIND_TX,
+       NETDEV_CMD_QUEUE_SET,

        __NETDEV_CMD_MAX,
        NETDEV_CMD_MAX = (__NETDEV_CMD_MAX - 1)
--
2.49.0

-- 
Thanks,
Mina
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help