| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHS8izNjZEZLqpXFNjFKQy8ngB8qOdSYVe_pG_vggS8iPGJvVg@mail.gmail.com>
Date: Tue, 19 Aug 2025 15:36:04 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
Eric Dumazet <edumazet@...gle.com>, Willem de Bruijn <willemb@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, andrew+netdev@...n.ch, horms@...nel.org,
davem@...emloft.net, sdf@...ichev.me, dw@...idwei.uk,
michael.chan@...adcom.com, dtatulea@...dia.com, ap420073@...il.com,
linux-kernel@...r.kernel.org, io-uring@...r.kernel.org
Subject: Re: [PATCH net-next v3 18/23] netdev: add support for setting
rx-buf-len per queue
On Mon, Aug 18, 2025 at 6:57 AM Pavel Begunkov <asml.silence@...il.com> wrote:
>
> From: Jakub Kicinski <kuba@...nel.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@...nel.org>
> Signed-off-by: Pavel Begunkov <asml.silence@...il.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.
> * @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.
> + 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?
> + 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
Powered by blists - more mailing lists