Large rx buffer support for zcrx

From: Jakub Kicinski <[email protected]>

Document the semantics of the rx_buf_len ethtool ring param. Clarify its meaning in case of HDS, where driver may have two separate buffer pools.

The various zero-copy TCP Rx schemes we have suffer from memory management overhead. Specifically applications aren't too impressed with the number of 4kB buffers they have to juggle. Zero-copy TCP makes most sense with larger memory transfers so using 16kB or 32kB buffers (with the help of HW-GRO) feels more natural.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 Documentation/networking/ethtool-netlink.rst | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Documentation/networking/ethtool-netlink.rst +0 -1

index b6e9af4d0f1b..eaa9c17a3cb1 100644
@@ -957,7 +957,6 @@ Kernel checks that requested ring sizes do not exceed limits reported by
 driver. Driver may impose additional constraints and may not support all
 attributes.
 
-
 ``ETHTOOL_A_RINGS_CQE_SIZE`` specifies the completion queue event size.
 Completion queue events (CQE) are the events posted by NIC to indicate the
 completion status of a packet when the packet is sent (like send success or

Documentation/networking/ethtool-netlink.rst +3 -0

+and payload this setting may control the size of the header buffers but must
+control the size of the payload buffers.
+

From: Jakub Kicinski <[email protected]>

Document the semantics of the rx_buf_len ethtool ring param. Clarify its meaning in case of HDS, where driver may have two separate buffer pools.

The various zero-copy TCP Rx schemes we have suffer from memory management overhead. Specifically applications aren't too impressed with the number of 4kB buffers they have to juggle. Zero-copy TCP makes most sense with larger memory transfers so using 16kB or 32kB buffers (with the help of HW-GRO) feels more natural.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 Documentation/networking/ethtool-netlink.rst | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Documentation/networking/ethtool-netlink.rst +0 -1

index b6e9af4d0f1b..eaa9c17a3cb1 100644
@@ -957,7 +957,6 @@ Kernel checks that requested ring sizes do not exceed limits reported by
 driver. Driver may impose additional constraints and may not support all
 attributes.
 
-
 ``ETHTOOL_A_RINGS_CQE_SIZE`` specifies the completion queue event size.
 Completion queue events (CQE) are the events posted by NIC to indicate the
 completion status of a packet when the packet is sent (like send success or

Documentation/networking/ethtool-netlink.rst completion queue size can be adjusted in the driver if CQE size is modified. · line 977

Quoted 5 lines

+``ETHTOOL_A_RINGS_RX_BUF_LEN`` controls the size of the buffer chunks driver

Documentation/networking/ethtool-netlink.rst +4 -0

+uses to receive packets. If the device uses different memory polls for headers
+and payload this setting may control the size of the header buffers but must
+control the size of the payload buffers.
+  

To be honest I'm not a big fan of the ambiguity here? Could this configure just the payload buffer sizes? And a new one to configure the header buffer sizes eventually?

Also, IIUC in this patchset, actually the size applied will be the order that is larger than the size configured, no? So a setting of 9KB will actually result in 16KB, no? Should this be documented? Or do we expect non power of 2 sizes to be rejected by the driver and this API fail?

From: Jakub Kicinski <[email protected]>

Unlike most of our APIs the rx-buf-len param does not have an associated max value. In theory user could set this value pretty high, but in practice most NICs have limits due to the width of the length fields in the descriptors.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 Documentation/netlink/specs/ethtool.yaml                  | 4 ++++
 Documentation/networking/ethtool-netlink.rst              | 1 +
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c | 3 ++-
 include/linux/ethtool.h                                   | 2 ++
 include/uapi/linux/ethtool_netlink_generated.h            | 1 +
 net/ethtool/rings.c                                       | 5 +++++
 6 files changed, 15 insertions(+), 1 deletion(-)

Documentation/netlink/specs/ethtool.yaml +4 -0

index 72a076b0e1b5..cb96b4e7093f 100644
@@ -361,6 +361,9 @@ attribute-sets:
       -
         name: hds-thresh-max
         type: u32
+      -
+        name: rx-buf-len-max
+        type: u32
 
   -
     name: mm-stat

@@ -1811,6 +1814,7 @@ operations:
             - rx-jumbo
             - tx
             - rx-buf-len
+            - rx-buf-len-max
             - tcp-data-split
             - cqe-size
             - tx-push

Documentation/networking/ethtool-netlink.rst +1 -0

index eaa9c17a3cb1..b7a99dfdffa9 100644
@@ -893,6 +893,7 @@ Kernel response contents:
   ``ETHTOOL_A_RINGS_RX_JUMBO``              u32     size of RX jumbo ring
   ``ETHTOOL_A_RINGS_TX``                    u32     size of TX ring
   ``ETHTOOL_A_RINGS_RX_BUF_LEN``            u32     size of buffers on the ring
+  ``ETHTOOL_A_RINGS_RX_BUF_LEN_MAX``        u32     max size of rx buffers
   ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT``        u8      TCP header / data split
   ``ETHTOOL_A_RINGS_CQE_SIZE``              u32     Size of TX/RX CQE
   ``ETHTOOL_A_RINGS_TX_PUSH``               u8      flag of TX Push mode

drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c +2 -1

index 45b8c9230184..7bdef64926c8 100644
@@ -376,6 +376,7 @@ static void otx2_get_ringparam(struct net_device *netdev,
 	ring->tx_max_pending = Q_COUNT(Q_SIZE_MAX);
 	ring->tx_pending = qs->sqe_cnt ? qs->sqe_cnt : Q_COUNT(Q_SIZE_4K);
 	kernel_ring->rx_buf_len = pfvf->hw.rbuf_len;
+	kernel_ring->rx_buf_len_max = 32768;
 	kernel_ring->cqe_size = pfvf->hw.xqe_size;
 }
 

@@ -398,7 +399,7 @@ static int otx2_set_ringparam(struct net_device *netdev,
 	/* Hardware supports max size of 32k for a receive buffer
 	 * and 1536 is typical ethernet frame size.
 	 */
-	if (rx_buf_len && (rx_buf_len < 1536 || rx_buf_len > 32768)) {
+	if (rx_buf_len && (rx_buf_len < 1536)) {
 		netdev_err(netdev,
 			   "Receive buffer range is 1536 - 32768");
 		return -EINVAL;

include/linux/ethtool.h +2 -0

index 5e0dd333ad1f..dd9f253a56ae 100644
@@ -77,6 +77,7 @@ enum {
 /**
  * struct kernel_ethtool_ringparam - RX/TX ring configuration
  * @rx_buf_len: Current length of buffers on the rx ring.
+ * @rx_buf_len_max: Max length of buffers on the rx ring.
  * @tcp_data_split: Scatter packet headers and data to separate buffers
  * @tx_push: The flag of tx push mode
  * @rx_push: The flag of rx push mode

@@ -89,6 +90,7 @@ enum {
  */
 struct kernel_ethtool_ringparam {
 	u32	rx_buf_len;
+	u32	rx_buf_len_max;
 	u8	tcp_data_split;
 	u8	tx_push;
 	u8	rx_push;

include/uapi/linux/ethtool_netlink_generated.h +1 -0

index aa8ab5227c1e..1a76e6789e33 100644
@@ -164,6 +164,7 @@ enum {
 	ETHTOOL_A_RINGS_TX_PUSH_BUF_LEN_MAX,
 	ETHTOOL_A_RINGS_HDS_THRESH,
 	ETHTOOL_A_RINGS_HDS_THRESH_MAX,
+	ETHTOOL_A_RINGS_RX_BUF_LEN_MAX,
 
 	__ETHTOOL_A_RINGS_CNT,
 	ETHTOOL_A_RINGS_MAX = (__ETHTOOL_A_RINGS_CNT - 1)

net/ethtool/rings.c +5 -0

index aeedd5ec6b8c..5e872ceab5dd 100644
@@ -105,6 +105,9 @@ static int rings_fill_reply(struct sk_buff *skb,
 			  ringparam->tx_pending)))  ||
 	    (kr->rx_buf_len &&
 	     (nla_put_u32(skb, ETHTOOL_A_RINGS_RX_BUF_LEN, kr->rx_buf_len))) ||
+	    (kr->rx_buf_len_max &&
+	     (nla_put_u32(skb, ETHTOOL_A_RINGS_RX_BUF_LEN_MAX,
+			  kr->rx_buf_len_max))) ||
 	    (kr->tcp_data_split &&
 	     (nla_put_u8(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT,
 			 kr->tcp_data_split))) ||

@@ -281,6 +284,8 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 		err_attr = tb[ETHTOOL_A_RINGS_TX];
 	else if (kernel_ringparam.hds_thresh > kernel_ringparam.hds_thresh_max)
 		err_attr = tb[ETHTOOL_A_RINGS_HDS_THRESH];
+	else if (kernel_ringparam.rx_buf_len > kernel_ringparam.rx_buf_len_max)
+		err_attr = tb[ETHTOOL_A_RINGS_RX_BUF_LEN];
 	else
 		err_attr = NULL;
 	if (err_attr) {

Signature · 1 line

2.49.0

From: Jakub Kicinski <[email protected]>

Distinguish between rx_buf_len being driver default vs user config. Use 0 as a special value meaning "unset" or "restore driver default". This will be necessary later on to configure it per-queue, but the ability to restore defaults may be useful in itself.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 Documentation/networking/ethtool-netlink.rst              | 2 +-
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c | 3 +++
 include/linux/ethtool.h                                   | 1 +
 net/ethtool/rings.c                                       | 2 +-
 4 files changed, 6 insertions(+), 2 deletions(-)

Documentation/networking/ethtool-netlink.rst +1 -1

index b7a99dfdffa9..723f8e1a33a7 100644
@@ -974,7 +974,7 @@ threshold value, header and data will be split.
 ``ETHTOOL_A_RINGS_RX_BUF_LEN`` controls the size of the buffer chunks driver
 uses to receive packets. If the device uses different memory polls for headers
 and payload this setting may control the size of the header buffers but must
-control the size of the payload buffers.
+control the size of the payload buffers. Setting to 0 restores driver default.
 
 CHANNELS_GET
 ============

drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c +3 -0

index 7bdef64926c8..1a74a7b81ac1 100644
@@ -396,6 +396,9 @@ static int otx2_set_ringparam(struct net_device *netdev,
 	if (ring->rx_mini_pending || ring->rx_jumbo_pending)
 		return -EINVAL;
 
+	if (!rx_buf_len)
+		rx_buf_len = OTX2_DEFAULT_RBUF_LEN;
+
 	/* Hardware supports max size of 32k for a receive buffer
 	 * and 1536 is typical ethernet frame size.
 	 */

include/linux/ethtool.h +1 -0

index dd9f253a56ae..bbc5c485bfbf 100644
@@ -77,6 +77,7 @@ enum {
 /**
  * struct kernel_ethtool_ringparam - RX/TX ring configuration
  * @rx_buf_len: Current length of buffers on the rx ring.
+ *		Setting to 0 means reset to driver default.
  * @rx_buf_len_max: Max length of buffers on the rx ring.
  * @tcp_data_split: Scatter packet headers and data to separate buffers
  * @tx_push: The flag of tx push mode

net/ethtool/rings.c +1 -1

index 5e872ceab5dd..628546a1827b 100644
@@ -139,7 +139,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
 	[ETHTOOL_A_RINGS_RX_MINI]		= { .type = NLA_U32 },
 	[ETHTOOL_A_RINGS_RX_JUMBO]		= { .type = NLA_U32 },
 	[ETHTOOL_A_RINGS_TX]			= { .type = NLA_U32 },
-	[ETHTOOL_A_RINGS_RX_BUF_LEN]            = NLA_POLICY_MIN(NLA_U32, 1),
+	[ETHTOOL_A_RINGS_RX_BUF_LEN]            = { .type = NLA_U32 },
 	[ETHTOOL_A_RINGS_TCP_DATA_SPLIT]	=
 		NLA_POLICY_MAX(NLA_U8, ETHTOOL_TCP_DATA_SPLIT_ENABLED),
 	[ETHTOOL_A_RINGS_CQE_SIZE]		= NLA_POLICY_MIN(NLA_U32, 1),

Signature · 1 line

2.49.0

From: Jakub Kicinski <[email protected]>

hds_thresh and hds_config are both inside struct netdev_config but have quite different semantics. hds_config is the user config with ternary semantics (on/off/unset). hds_thresh is a straight up value, populated by the driver at init and only modified by user space. We don't expect the drivers to have to pick a special hds_thresh value based on other configuration.

The two approaches have different advantages and downsides. hds_thresh ("direct value") gives core easy access to current device settings, but there's no way to express whether the value comes from the user. It also requires the initialization by the driver.

hds_config ("user config values") tells us what user wanted, but doesn't give us the current value in the core.

Try to explain this a bit in the comments, so at we make a conscious choice for new values which semantics we expect.

Move the init inside ethtool_ringparam_get_cfg() to reflect the semantics. Commit 216a61d33c07 ("net: ethtool: fix ethtool_ringparam_get_cfg() returns a hds_thresh value always as 0.") added the setting for the benefit of netdevsim which doesn't touch the value at all on get. Again, this is just to clarify the intention, shouldn't cause any functional change.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/net/netdev_queues.h | 19 +++++++++++++++++--
 net/ethtool/common.c        |  3 ++-
 2 files changed, 19 insertions(+), 3 deletions(-)

include/net/netdev_queues.h +0 -0

index ba2eaf39089b..81df0794d84c 100644
@@ -6,11 +6,26 @@
 
 /**
  * struct netdev_config - queue-related configuration for a netdev

include/net/netdev_queues.h line 16

Quoted 7 lines

struct netdev_config {

include/net/netdev_queues.h +6 -0

+       /* Direct value
+        *
+        * Driver default is expected to be fixed, and set in this struct
+        * at init. From that point on user may change the value. There is
+        * no explicit way to "unset" / restore driver default.
+        */  

Does the user setting hds_thres imply turning hds_config to "on"? Or is hds_thres only used when hds_config is actually on?

From: Jakub Kicinski <[email protected]>

Add rx_buf_len to configuration maintained by the core. Use "three-state" semantics where 0 means "driver default".

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/net/netdev_queues.h | 4 ++++
 net/ethtool/common.c        | 1 +
 net/ethtool/rings.c         | 2 ++
 3 files changed, 7 insertions(+)

include/net/netdev_queues.h +4 -0

index 81df0794d84c..eb3a5ac823e6 100644
@@ -24,6 +24,10 @@ struct netdev_config {
         * If "unset" driver is free to decide, and may change its choice
         * as other parameters change.
         */
+       /** @rx_buf_len: Size of buffers on the Rx ring
+        *               (ETHTOOL_A_RINGS_RX_BUF_LEN).
+        */
+       u32     rx_buf_len;
        /** @hds_config: HDS enabled (ETHTOOL_A_RINGS_TCP_DATA_SPLIT).
         */
        u8      hds_config;

net/ethtool/common.c +0 -0

index a87298f659f5..8fdffc77e981 100644

Quoted 2 lines · Pavel Begunkov

Add rx_buf_len to configuration maintained by the core. Use "three-state" semantics where 0 means "driver default".

What are three states in the semantics here?

- 0 = driver default.
- non-zero means value set by userspace

What is the 3rd state here?

net/ethtool/common.c void ethtool_ringparam_get_cfg(struct net_device *dev, · line 835

Quoted 9 lines

net/ethtool/common.c +1 -0

index a87298f659f5..8fdffc77e981 100644
@@ -832,6 +832,7 @@ void ethtool_ringparam_get_cfg(struct net_device *dev,
 
        /* Driver gives us current state, we want to return current config */
        kparam->tcp_data_split = dev->cfg->hds_config;
+       kparam->rx_buf_len = dev->cfg->rx_buf_len;  

I'm confused that struct netdev_config is defined in netdev_queues.h, and is documented to be a queue-related configuration, but doesn't seem to be actually per queue? This line is grabbing the current config for this queue from dev->cfg which looks like a shared value.

I don't think rx_buf_len should be a shared value between all the queues. I strongly think it should a per-queue value. The devmem/io_uring queues will probably want large rx_buf_len, but normal queues will want 0 buf len, me thinks.

From: Jakub Kicinski <[email protected]>

I imagine (tm) that as the number of per-queue configuration options grows some of them may conflict for certain drivers. While the drivers can obviously do all the validation locally doing so is fairly inconvenient as the config is fed to drivers piecemeal via different ops (for different params and NIC-wide vs per-queue).

Add a centralized callback for validating the queue config in queue ops. The callback gets invoked before each queue restart and when ring params are modified.

For NIC-wide changes the callback gets invoked for each active (or active to-be) queue, and additionally with a negative queue index for NIC-wide defaults. The NIC-wide check is needed in case all queues have an override active when NIC-wide setting is changed to an unsupported one. Alternatively we could check the settings when new queues are enabled (in the channel API), but accepting invalid config is a bad idea. Users may expect that resetting a queue override will always work.

The "trick" of passing a negative index is a bit ugly, we may want to revisit if it causes confusion and bugs. Existing drivers don't care about the index so it "just works".

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/net/netdev_queues.h | 12 ++++++++++++
 net/core/dev.h              |  2 ++
 net/core/netdev_config.c    | 20 ++++++++++++++++++++
 net/core/netdev_rx_queue.c  |  6 ++++++
 net/ethtool/rings.c         |  5 +++++
 5 files changed, 45 insertions(+)

include/net/netdev_queues.h +12 -0

index e3e7ecf91bac..f75313fc78ba 100644
@@ -146,6 +146,14 @@ void netdev_stat_queue_sum(struct net_device *netdev,
  *                     defaults. Queue config structs are passed to this
  *                     helper before the user-requested settings are applied.
  *
+ * @ndo_queue_cfg_validate: (Optional) Check if queue config is supported.
+ *                     Called when configuration affecting a queue may be
+ *                     changing, either due to NIC-wide config, or config
+ *                     scoped to the queue at a specified index.
+ *                     When NIC-wide config is changed the callback will
+ *                     be invoked for all queues, and in addition to that
+ *                     with a negative queue index for the base settings.
+ *
  * @ndo_queue_mem_alloc: Allocate memory for an RX queue at the specified index.
  *                      The new memory is written at the specified address.
  *

@@ -166,6 +174,10 @@ struct netdev_queue_mgmt_ops {
        void    (*ndo_queue_cfg_defaults)(struct net_device *dev,
                                          int idx,
                                          struct netdev_queue_config *qcfg);
+       int     (*ndo_queue_cfg_validate)(struct net_device *dev,
+                                         int idx,
+                                         struct netdev_queue_config *qcfg,
+                                         struct netlink_ext_ack *extack);
        int     (*ndo_queue_mem_alloc)(struct net_device *dev,
                                       struct netdev_queue_config *qcfg,
                                       void *per_queue_mem,

net/core/dev.h +2 -0

index 6d7f5e920018..e0d433fb6325 100644
@@ -99,6 +99,8 @@ void netdev_free_config(struct net_device *dev);
 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);
+int netdev_queue_config_revalidate(struct net_device *dev,
+                                  struct netlink_ext_ack *extack);
 
 /* netdev management, shared between various uAPI entry points */
 struct netdev_name_node {

net/core/netdev_config.c +1 -0

index bad2d53522f0..fc700b77e4eb 100644
@@ -99,3 +99,23 @@ void netdev_queue_config(struct net_device *dev, int rxq,
        __netdev_queue_config(dev, rxq, qcfg, true);
 }
 EXPORT_SYMBOL(netdev_queue_config);
+

net/core/netdev_config.c +9 -0

+       for (i = -1; i < (int)dev->real_num_rx_queues; i++) {
+               netdev_queue_config(dev, i, &qcfg);
+               err = qops->ndo_queue_cfg_validate(dev, i, &qcfg, extack);
+               if (err)
+                       return err;
+       }
+
+       return 0;
+}

net/core/netdev_rx_queue.c +6 -0

index b0523eb44e10..7c691eb1a48b 100644
@@ -37,6 +37,12 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx,
 
        netdev_queue_config(dev, rxq_idx, &qcfg);
 
+       if (qops->ndo_queue_cfg_validate) {
+               err = qops->ndo_queue_cfg_validate(dev, rxq_idx, &qcfg, extack);
+               if (err)
+                       goto err_free_old_mem;
+       }
+
        err = qops->ndo_queue_mem_alloc(dev, &qcfg, new_mem, rxq_idx);
        if (err)
                goto err_free_old_mem;

net/ethtool/rings.c +5 -1

index 6a74e7e4064e..7884d10c090f 100644
@@ -4,6 +4,7 @@
 
 #include "netlink.h"
 #include "common.h"
+#include "../core/dev.h"
 
 struct rings_req_info {
        struct ethnl_req_info           base;

@@ -307,6 +308,10 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
        dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
        dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
 
+       ret = netdev_queue_config_revalidate(dev, info->extack);
+       if (ret)
+               return ret;
+
        ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
                                              &kernel_ringparam, info->extack);
        return ret < 0 ? ret : 1;
--

2.49.0

From: Jakub Kicinski <[email protected]>

In normal operation only a subset of queues is configured for zero-copy. Since zero-copy is the main use for larger buffer sizes we need to configure the sizes per queue.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Quoted 8 lines

From: Jakub Kicinski <[email protected]>

In normal operation only a subset of queues is configured for zero-copy. Since zero-copy is the main use for larger buffer sizes we need to configure the sizes per queue.

Signed-off-by Jakub Kicinski <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

I wonder if this is necessary for some reason, or is it better to expect the driver to refer to the netdev->qcfgs directly?

By my count the configs can now live in 4 places: the core netdev config, the core per-queue config, the driver netdev config, and the driver per-queue config.

I honestly I'm not sure about duplicating settings between the netdev configs and the per-queue configs in the first place (seems like configs should be either driver wide or per-queue to me, and not both), and I'm less sure about again duplicating the settings between core structs and in-driver structs. Seems like the same information duplicated in many places and a nightmare to keep it all in sync.

From: Jakub Kicinski <[email protected]>

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 <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 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(+)

Documentation/netlink/specs/netdev.yaml +15 -0

index c0ef6d0d7786..5dd1eb5909cd 100644
@@ -324,6 +324,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: |

@@ -755,6 +759,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"]

include/net/netdev_queues.h +1 -0

index f75313fc78ba..cfd2d59861e1 100644
@@ -38,6 +38,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 */

I don't see this used anywhere.

But more generally, I'm a bit concerned about protecting drivers that don't support configuring one particular queue config. I think likely supported_ring_params needs to be moved earlier to the patch which adds per queue netdev_configs to the queue API, and probably as part of that patch core needs to make sure it's never asking a driver that doesn't support changing a netdev_queue_config to do so?

Some thought may be given to moving the entire configuration story outside of queue_mem_alloc/free queue_start/stop altogether to new ndos where core can easily check if the ndo is supported otherwise per-queue config is not supported. Otherwise core needs to be careful never to attempt a config that is not supported?

This patch allows memory providers to pass a queue config when opening a queue. It'll be used in the next patch to pass a custom rx buffer length from zcrx. As there are many users of netdev_rx_queue_restart(), it's allowed to pass a NULL qcfg, in which case the function will use the default configuration.

Quoted 5 lines · On Mon, 28 Jul 2025 12:04:25 +0100 Pavel Begunkov wrote:

This patch allows memory providers to pass a queue config when opening a queue. It'll be used in the next patch to pass a custom rx buffer length from zcrx. As there are many users of netdev_rx_queue_restart(), it's allowed to pass a NULL qcfg, in which case the function will use the default configuration.

This is not exactly what I anticipated, TBH, I was thinking of extending the config stuff with another layer.. Drivers will restart their queues for most random reasons, so we need to be able to reconstitute this config easily and serve it up via

netdev_queue_config(). This was, IIUC, also Mina's first concern.

My thinking was that the config would be constructed like this:

qcfg = init_to_defaults()
drv_def = get_driver_defaults()
for each setting:
if drv_def.X.set:
qcfg.X = drv_def.X.value
if dev.config.X.set:
qcfg.X = dev.config.X.value
if dev.config.qcfg[qid].X.set:
qcfg.X = dev.config.qcfg[qid].X.value
if dev.config.mp[qid].X.set: << this was not in my
qcfg.X = dev.config.mp[qid].X.value << RFC series

Since we don't allow MP to be replaced atomically today, we don't actually have to place the mp overrides in the config struct and involve the whole netdev_reconfig_start() _swap() _free() machinery. We can just stash the config in the queue state, and "logically" do what I described above.

Quoted 5 lines

Since we don't allow MP to be replaced atomically today, we don't actually have to place the mp overrides in the config struct and involve the whole netdev_reconfig_start() _swap() _free() machinery. We can just stash the config in the queue state, and "logically" do what I described above.

I was thinking stashing it in struct pp_memory_provider_params and applying in netdev_rx_queue_restart(). Let me try to move it into __netdev_queue_config. Any preference between keeping just the size vs a qcfg pointer in pp_memory_provider_params?

struct struct pp_memory_provider_params {
	const struct memory_provider_ops *mp_ops;
	u32 rx_buf_len;
};

vs

struct struct pp_memory_provider_params {
	const struct memory_provider_ops *mp_ops;
	// providers will need to allocate and keep the qcfg
	// until it's completely detached from the queues.
	struct netdev_queue_config *qcfg;
};

The former one would be simpler for now.

I was thinking stashing it in struct pp_memory_provider_params and applying in netdev_rx_queue_restart().

Quoted 11 lines · Jakub Kicinski

Quoted 5 lines · On Mon, 28 Jul 2025 12:04:25 +0100 Pavel Begunkov wrote:

This patch allows memory providers to pass a queue config when opening a queue. It'll be used in the next patch to pass a custom rx buffer length from zcrx. As there are many users of netdev_rx_queue_restart(), it's allowed to pass a NULL qcfg, in which case the function will use the default configuration.

This is not exactly what I anticipated, TBH, I was thinking of extending the config stuff with another layer.. Drivers will restart their queues for most random reasons, so we need to be able to reconstitute this config easily and serve it up via

Yeah, also noticed the gap that while replying to Stan.

Quoted 16 lines

netdev_queue_config(). This was, IIUC, also Mina's first concern.

My thinking was that the config would be constructed like this:

qcfg = init_to_defaults()
drv_def = get_driver_defaults()
for each setting:
if drv_def.X.set:
qcfg.X = drv_def.X.value
if dev.config.X.set:
qcfg.X = dev.config.X.value
if dev.config.qcfg[qid].X.set:
qcfg.X = dev.config.qcfg[qid].X.value
if dev.config.mp[qid].X.set: << this was not in my
qcfg.X = dev.config.mp[qid].X.value << RFC series

Quoted 5 lines

Since we don't allow MP to be replaced atomically today, we don't actually have to place the mp overrides in the config struct and involve the whole netdev_reconfig_start() _swap() _free() machinery. We can just stash the config in the queue state, and "logically" do what I described above.

I was thinking stashing it in struct pp_memory_provider_params and applying in netdev_rx_queue_restart(). Let me try to move it into __netdev_queue_config. Any preference between keeping just the size vs a qcfg pointer in pp_memory_provider_params?

struct struct pp_memory_provider_params {
        const struct memory_provider_ops *mp_ops;
        u32 rx_buf_len;
};

iouring zcrx and devmem can configure netdev_rx_queue->rx_buf_len in addition to netdev_rx_queue->mp_params in this scenario.

Quoted 2 lines · On Wed, 6 Aug 2025 09:48:56 -0700 Mina Almasry wrote:

iouring zcrx and devmem can configure netdev_rx_queue->rx_buf_len in addition to netdev_rx_queue->mp_params in this scenario.

Did you not read my message or are you disagreeing that the setting should be separate and form a hierarchy?

Sorry, I was disagreeing. The flow above seems complicated. I'm probably missing something that requires this complication. I was suggesting an approach I find more straightforward.

Something like:
nedev_config = get_driver_defaults()
qcfg = get_driver_defaults()

for each setting:
if qcfg[i].X is set:
use qcfg[i].X
else
use netdev_config.X

This series implements large rx buffer support for io_uring/zcrx on top of Jakub's queue configuration changes, but it can also be used by other memory providers. Large rx buffers can be drastically beneficial with high-end hw-gro enabled cards that can coalesce traffic into larger pages, reducing the number of frags traversing the network stack and resuling in larger contiguous chunks of data for the userspace. Benchamrks showed up to ~30% improvement in CPU util.

For example, for 200Gbit broadcom NIC, 4K vs 32K buffers, and napi and userspace pinned to the same CPU:

packets=23987040 (MB=2745098), rps=199559 (MB/s=22837)

CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
  0    1.53    0.00   27.78    2.72    1.31   66.45    0.22
packets=24078368 (MB=2755550), rps=200319 (MB/s=22924)
CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
  0    0.69    0.00    8.26   31.65    1.83   57.00    0.57

And for napi and userspace on different CPUs:

packets=10725082 (MB=1227388), rps=198285 (MB/s=22692)

CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
  0    0.10    0.00    0.50    0.00    0.50   74.50    24.40
  1    4.51    0.00   44.33   47.22    2.08    1.85    0.00
packets=14026235 (MB=1605175), rps=198388 (MB/s=22703)
CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
  0    0.10    0.00    0.70    0.00    1.00   43.78   54.42
  1    1.09    0.00   31.95   62.91    1.42    2.63    0.00

Patch 19 allows to pass queue config from a memory provider. The zcrx changes are contained in a single patch as I already queued most of work making it size agnostic into my zcrx branch. The uAPI is simple and imperative, it'll use the exact value (if) specified by the user. In the future we might extend it to "choose the best size in a given range".

The rest (first 20) patches are from Jakub's series implementing per queue configuration. Quoting Jakub:

"... The direct motivation for the series is that zero-copy Rx queues would like to use larger Rx buffers. Most modern high-speed NICs support HW-GRO, and can coalesce payloads into pages much larger than than the MTU. Enabling larger buffers globally is a bit precarious as it exposes us to potentially very inefficient memory use. Also allocating large buffers may not be easy or cheap under load. Zero-copy queues service only select traffic and have pre-allocated memory so the concerns don't apply as much.

The per-queue config has to address 3 problems:
- user API
- driver API
- memory provider API

For user API the main question is whether we expose the config via ethtool or netdev nl. I picked the latter - via queue GET/SET, rather than extending the ethtool RINGS_GET API. I worry slightly that queue GET/SET will turn in a monster like SETLINK. OTOH the only per-queue settings we have in ethtool which are not going via RINGS_SET is IRQ coalescing.

My goal for the driver API was to avoid complexity in the drivers. The queue management API has gained two ops, responsible for preparing configuration for a given queue, and validating whether the config is supported. The validating is used both for NIC-wide and per-queue changes. Queue alloc/start ops have a new "config" argument which contains the current config for a given queue (we use queue restart to apply per-queue settings). Outside of queue reset paths drivers can call netdev_queue_config() which returns the config for an arbitrary queue. Long story short I anticipate it to be used during ndo_open.

In the core I extended struct netdev_config with per queue settings. All in all this isn't too far from what was there in my "queue API prototype" a few years ago ..."

Quoted 70 lines · Pavel Begunkov

This series implements large rx buffer support for io_uring/zcrx on top of Jakub's queue configuration changes, but it can also be used by other memory providers. Large rx buffers can be drastically beneficial with high-end hw-gro enabled cards that can coalesce traffic into larger pages, reducing the number of frags traversing the network stack and resuling in larger contiguous chunks of data for the userspace. Benchamrks showed up to ~30% improvement in CPU util.

For example, for 200Gbit broadcom NIC, 4K vs 32K buffers, and napi and userspace pinned to the same CPU:

packets=23987040 (MB=2745098), rps=199559 (MB/s=22837)
CPU %usr %nice %sys %iowait %irq %soft %idle
0 1.53 0.00 27.78 2.72 1.31 66.45 0.22
packets=24078368 (MB=2755550), rps=200319 (MB/s=22924)
CPU %usr %nice %sys %iowait %irq %soft %idle
0 0.69 0.00 8.26 31.65 1.83 57.00 0.57

And for napi and userspace on different CPUs:

packets=10725082 (MB=1227388), rps=198285 (MB/s=22692)
CPU %usr %nice %sys %iowait %irq %soft %idle

   0    0.10    0.00    0.50    0.00    0.50   74.50    24.40
   1    4.51    0.00   44.33   47.22    2.08    1.85    0.00
packets=14026235 (MB=1605175), rps=198388 (MB/s=22703)
CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
   0    0.10    0.00    0.70    0.00    1.00   43.78   54.42
   1    1.09    0.00   31.95   62.91    1.42    2.63    0.00

Patch 19 allows to pass queue config from a memory provider. The zcrx changes are contained in a single patch as I already queued most of work making it size agnostic into my zcrx branch. The uAPI is simple and imperative, it'll use the exact value (if) specified by the user. In the future we might extend it to "choose the best size in a given range".

The rest (first 20) patches are from Jakub's series implementing per queue configuration. Quoting Jakub:

"... The direct motivation for the series is that zero-copy Rx queues would like to use larger Rx buffers. Most modern high-speed NICs support HW-GRO, and can coalesce payloads into pages much larger than than the MTU. Enabling larger buffers globally is a bit precarious as it exposes us to potentially very inefficient memory use. Also allocating large buffers may not be easy or cheap under load. Zero-copy queues service only select traffic and have pre-allocated memory so the concerns don't apply as much.

The per-queue config has to address 3 problems:
- user API
- driver API
- memory provider API

For user API the main question is whether we expose the config via ethtool or netdev nl. I picked the latter - via queue GET/SET, rather than extending the ethtool RINGS_GET API. I worry slightly that queue GET/SET will turn in a monster like SETLINK. OTOH the only per-queue settings we have in ethtool which are not going via RINGS_SET is IRQ coalescing.

My goal for the driver API was to avoid complexity in the drivers. The queue management API has gained two ops, responsible for preparing configuration for a given queue, and validating whether the config is supported. The validating is used both for NIC-wide and per-queue changes. Queue alloc/start ops have a new "config" argument which contains the current config for a given queue (we use queue restart to apply per-queue settings). Outside of queue reset paths drivers can call netdev_queue_config() which returns the config for an arbitrary queue. Long story short I anticipate it to be used during ndo_open.

Quoted 3 lines

In the core I extended struct netdev_config with per queue settings. All in all this isn't too far from what was there in my "queue API prototype" a few years ago ..."

Supporting big buffers is the right direction, but I have the same feedback:

We should also aim for another use-case where we allocate page pool chunks from the huge page(s),

We need some way to express these things from the UAPI point of view.

Flipping the rx-buf-len value seems too fragile - there needs to be something to request 32K chunks only for devmem case, not for the (default) CPU memory. And the queues should go back to default 4K pages when the dmabuf is detached from the queue.

Quoted 71 lines · Stanislav Fomichev

Quoted 70 lines · Pavel Begunkov

This series implements large rx buffer support for io_uring/zcrx on top of Jakub's queue configuration changes, but it can also be used by other memory providers. Large rx buffers can be drastically beneficial with high-end hw-gro enabled cards that can coalesce traffic into larger pages, reducing the number of frags traversing the network stack and resuling in larger contiguous chunks of data for the userspace. Benchamrks showed up to ~30% improvement in CPU util.

For example, for 200Gbit broadcom NIC, 4K vs 32K buffers, and napi and userspace pinned to the same CPU:

packets=23987040 (MB=2745098), rps=199559 (MB/s=22837)
CPU %usr %nice %sys %iowait %irq %soft %idle
0 1.53 0.00 27.78 2.72 1.31 66.45 0.22
packets=24078368 (MB=2755550), rps=200319 (MB/s=22924)
CPU %usr %nice %sys %iowait %irq %soft %idle
0 0.69 0.00 8.26 31.65 1.83 57.00 0.57

And for napi and userspace on different CPUs:

packets=10725082 (MB=1227388), rps=198285 (MB/s=22692)
CPU %usr %nice %sys %iowait %irq %soft %idle

   0    0.10    0.00    0.50    0.00    0.50   74.50    24.40
   1    4.51    0.00   44.33   47.22    2.08    1.85    0.00
packets=14026235 (MB=1605175), rps=198388 (MB/s=22703)
CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
   0    0.10    0.00    0.70    0.00    1.00   43.78   54.42
   1    1.09    0.00   31.95   62.91    1.42    2.63    0.00

Patch 19 allows to pass queue config from a memory provider. The zcrx changes are contained in a single patch as I already queued most of work making it size agnostic into my zcrx branch. The uAPI is simple and imperative, it'll use the exact value (if) specified by the user. In the future we might extend it to "choose the best size in a given range".

The rest (first 20) patches are from Jakub's series implementing per queue configuration. Quoting Jakub:

"... The direct motivation for the series is that zero-copy Rx queues would like to use larger Rx buffers. Most modern high-speed NICs support HW-GRO, and can coalesce payloads into pages much larger than than the MTU. Enabling larger buffers globally is a bit precarious as it exposes us to potentially very inefficient memory use. Also allocating large buffers may not be easy or cheap under load. Zero-copy queues service only select traffic and have pre-allocated memory so the concerns don't apply as much.

The per-queue config has to address 3 problems:
- user API
- driver API
- memory provider API

For user API the main question is whether we expose the config via ethtool or netdev nl. I picked the latter - via queue GET/SET, rather than extending the ethtool RINGS_GET API. I worry slightly that queue GET/SET will turn in a monster like SETLINK. OTOH the only per-queue settings we have in ethtool which are not going via RINGS_SET is IRQ coalescing.

My goal for the driver API was to avoid complexity in the drivers. The queue management API has gained two ops, responsible for preparing configuration for a given queue, and validating whether the config is supported. The validating is used both for NIC-wide and per-queue changes. Queue alloc/start ops have a new "config" argument which contains the current config for a given queue (we use queue restart to apply per-queue settings). Outside of queue reset paths drivers can call netdev_queue_config() which returns the config for an arbitrary queue. Long story short I anticipate it to be used during ndo_open.

Quoted 6 lines

Quoted 3 lines

In the core I extended struct netdev_config with per queue settings. All in all this isn't too far from what was there in my "queue API prototype" a few years ago ..."

Supporting big buffers is the right direction, but I have the same feedback:

Let me actually check the feedback for the queue config RFC...

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Quoted 2 lines

We should also aim for another use-case where we allocate page pool chunks from the huge page(s),

Separate huge page pool is a bit beyond the scope of this series.

this should push the perf even more.

And not sure about "even more" is from, you can already register a huge page with zcrx, and this will allow to chunk them to 32K or so for hardware. Is it in terms of applicability or you have some perf optimisation ideas?

Quoted 1 line

We need some way to express these things from the UAPI point of view.

Can you elaborate?

Quoted 4 lines

Flipping the rx-buf-len value seems too fragile - there needs to be something to request 32K chunks only for devmem case, not for the (default) CPU memory. And the queues should go back to default 4K pages when the dmabuf is detached from the queue.

That's what the per-queue config is solving. It's not default, zcrx configures it only for the specific queue it allocated, and the value is cleared on restart in netdev_rx_queue_restart(), if not even too aggressively. Maybe I should just stash it into mp_params to make sure it's not cleared if a provider is still attached on a spurious restart.

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

care is exposing it via io_uring, maybe don't expose it from netlink for

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

Quoted 7 lines

Quoted 2 lines

We should also aim for another use-case where we allocate page pool chunks from the huge page(s),

Separate huge page pool is a bit beyond the scope of this series.

this should push the perf even more.

Quoted 4 lines

And not sure about "even more" is from, you can already register a huge page with zcrx, and this will allow to chunk them to 32K or so for hardware. Is it in terms of applicability or you have some perf optimisation ideas?

What I'm looking for is a generic system-wide solution where we can set up the host to use huge pages to back all (even non-zc) networking queues. Not necessary needed, but might be an option to try.

Quoted 9 lines

Quoted 1 line

We need some way to express these things from the UAPI point of view.

Can you elaborate?

Quoted 4 lines

Flipping the rx-buf-len value seems too fragile - there needs to be something to request 32K chunks only for devmem case, not for the (default) CPU memory. And the queues should go back to default 4K pages when the dmabuf is detached from the queue.

Quoted 6 lines

That's what the per-queue config is solving. It's not default, zcrx configures it only for the specific queue it allocated, and the value is cleared on restart in netdev_rx_queue_restart(), if not even too aggressively. Maybe I should just stash it into mp_params to make sure it's not cleared if a provider is still attached on a spurious restart.

If we assume that at some point niov can be backed up by chunks larger than PAGE_SIZE, the assumed workflow for devemem is:
1. change rx-buf-len to 32K
- this is needed only for devmem, but not for CPU RAM, but we'll have to refill the queues from the main memory anyway

- there is also a question on whether we need to do anything about MAX_PAGE_ORDER/PAGE_ALLOC_COSTLY_ORDER - do we just let the driver allocations fail?
2. attach dmabuf to the queue to refill from dmabuf sgt, essentially wasting all the effort on (1)
3. on detach, something needs to also not forget to reset the rx-buf-len back to PAGE_SIZE

I was hoping that maybe we can bind rx-buf-len to dmabuf for devmem, that should avoid all that useless refill from the main memory with large chunks. But I'm not sure it's the right way to go either.

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

We should also aim for another use-case where we allocate page pool chunks from the huge page(s),

Separate huge page pool is a bit beyond the scope of this series.

this should push the perf even more.

Quoted 8 lines

Quoted 4 lines

And not sure about "even more" is from, you can already register a huge page with zcrx, and this will allow to chunk them to 32K or so for hardware. Is it in terms of applicability or you have some perf optimisation ideas?

What I'm looking for is a generic system-wide solution where we can set up the host to use huge pages to back all (even non-zc) networking queues. Not necessary needed, but might be an option to try.

Probably like what Jakub was once suggesting with the initial memory provider patch, got it.

Quoted 16 lines

Quoted 9 lines

Quoted 1 line

We need some way to express these things from the UAPI point of view.

Can you elaborate?

Quoted 4 lines

Flipping the rx-buf-len value seems too fragile - there needs to be something to request 32K chunks only for devmem case, not for the (default) CPU memory. And the queues should go back to default 4K pages when the dmabuf is detached from the queue.

Quoted 6 lines

That's what the per-queue config is solving. It's not default, zcrx configures it only for the specific queue it allocated, and the value is cleared on restart in netdev_rx_queue_restart(), if not even too aggressively. Maybe I should just stash it into mp_params to make sure it's not cleared if a provider is still attached on a spurious restart.

Quoted 5 lines

If we assume that at some point niov can be backed up by chunks larger than PAGE_SIZE, the assumed workflow for devemem is:
1. change rx-buf-len to 32K
- this is needed only for devmem, but not for CPU RAM, but we'll have to refill the queues from the main memory anyway

Urgh, that's another reason why I prefer to just pass it through zcrx and not netlink. So maybe you can just pass the len to devmem on creation, and internally it sets up its queues with it.

Quoted 28 lines · Pavel Begunkov

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

Quoted 5 lines

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Don't you already need a bunch of netlink to setup rss and flow

steering? And if we end up adding queue api, you'll have to call that one over netlink also.

Quoted 7 lines

Quoted 6 lines

If we assume that at some point niov can be backed up by chunks larger than PAGE_SIZE, the assumed workflow for devemem is:
1. change rx-buf-len to 32K
- this is needed only for devmem, but not for CPU RAM, but we'll have to refill the queues from the main memory anyway

Quoted 3 lines

Urgh, that's another reason why I prefer to just pass it through zcrx and not netlink. So maybe you can just pass the len to devmem on creation, and internally it sets up its queues with it.

But you still need to solve MAX_PAGE_ORDER/PAGE_ALLOC_COSTLY_ORDER I think? We don't want the drivers to do PAGE_ALLOC_COSTLY_ORDER costly allocation presumably?

Quoted 35 lines · Stanislav Fomichev

Quoted 33 lines · Pavel Begunkov

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Quoted 1 line

Don't you already need a bunch of netlink to setup rss and flow

Could be needed, but there are cases where configuration and virtual queue selection is done outside the program. I'll need to ask which option we currently use.

Quoted 2 lines

steering? And if we end up adding queue api, you'll have to call that one over netlink also.

There is already a queue api, even though it's cropped IIUC. What kind of extra setup you have in mind?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

If we assume that at some point niov can be backed up by chunks larger than PAGE_SIZE, the assumed workflow for devemem is:
1. change rx-buf-len to 32K
- this is needed only for devmem, but not for CPU RAM, but we'll have to refill the queues from the main memory anyway

Quoted 7 lines

Quoted 3 lines

Urgh, that's another reason why I prefer to just pass it through zcrx and not netlink. So maybe you can just pass the len to devmem on creation, and internally it sets up its queues with it.

But you still need to solve MAX_PAGE_ORDER/PAGE_ALLOC_COSTLY_ORDER I think? We don't want the drivers to do PAGE_ALLOC_COSTLY_ORDER costly allocation presumably?

#define PAGE_ALLOC_COSTLY_ORDER 3

It's "costly" for the page allocator and not a custom specially cooked memory providers. Nobody should care as long as the length applies to the given provider only. MAX_PAGE_ORDER also seems to be a page allocator thing.

Quoted 36 lines · Pavel Begunkov

Quoted 35 lines · Stanislav Fomichev

Quoted 33 lines · Pavel Begunkov

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Quoted 5 lines

Quoted 1 line

Don't you already need a bunch of netlink to setup rss and flow

Could be needed, but there are cases where configuration and virtual queue selection is done outside the program. I'll need to ask which option we currently use.

If the setup is done outside, you can also setup rx-buf-len outside, no?

Quoted 5 lines

Quoted 2 lines

steering? And if we end up adding queue api, you'll have to call that one over netlink also.

There is already a queue api, even though it's cropped IIUC. What kind of extra setup you have in mind?

I'm talking about allocating the queues. Currently the zc/devmem setup is a bit complicated, we need to partition the queues and rss+flow steer into a subset of zerocopy ones. In the future we might add some apis to request a new dedicated queue for the specific flow(s). That should hopefully simplify the design (and make the cleanup of the queues more robust if the application dies).

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

Quoted 5 lines

If we assume that at some point niov can be backed up by chunks larger than PAGE_SIZE, the assumed workflow for devemem is:
1. change rx-buf-len to 32K
- this is needed only for devmem, but not for CPU RAM, but we'll have to refill the queues from the main memory anyway

Urgh, that's another reason why I prefer to just pass it through zcrx and not netlink. So maybe you can just pass the len to devmem on creation, and internally it sets up its queues with it.

But you still need to solve MAX_PAGE_ORDER/PAGE_ALLOC_COSTLY_ORDER I think? We don't want the drivers to do PAGE_ALLOC_COSTLY_ORDER costly allocation presumably?

Quoted 6 lines

#define PAGE_ALLOC_COSTLY_ORDER 3

It's "costly" for the page allocator and not a custom specially cooked memory providers. Nobody should care as long as the length applies to the given provider only. MAX_PAGE_ORDER also seems to be a page allocator thing.

By custom memory providers you mean page pool? Thinking about it more,

maybe it's fine as is as long as we have ndo_queue_cfg_validate that enforces sensible ranges..

Quoted 43 lines · Stanislav Fomichev

Quoted 41 lines · Pavel Begunkov

Quoted 29 lines · Stanislav Fomichev

Quoted 28 lines · Pavel Begunkov

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

Quoted 7 lines

Quoted 5 lines

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Don't you already need a bunch of netlink to setup rss and flow

Could be needed, but there are cases where configuration and virtual queue selection is done outside the program. I'll need to ask which option we currently use.

Quoted 1 line

If the setup is done outside, you can also setup rx-buf-len outside, no?

You can't do it without assuming the memory layout, and that's the application's role to allocate buffers. Not to mention that often the app won't know about all specifics either and it'd be resolved on zcrx registration.

Quoted 44 lines · Pavel Begunkov

Quoted 43 lines · Stanislav Fomichev

Quoted 41 lines · Pavel Begunkov

Quoted 29 lines · Stanislav Fomichev

Quoted 28 lines · Pavel Begunkov

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

Quoted 7 lines

Quoted 5 lines

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Don't you already need a bunch of netlink to setup rss and flow

Could be needed, but there are cases where configuration and virtual queue selection is done outside the program. I'll need to ask which option we currently use.

Quoted 6 lines

Quoted 1 line

If the setup is done outside, you can also setup rx-buf-len outside, no?

You can't do it without assuming the memory layout, and that's the application's role to allocate buffers. Not to mention that often the app won't know about all specifics either and it'd be resolved on zcrx registration.

I think, fundamentally, we need to distinguish:

1. chunk size of the memory pool (page pool order, niov size)
2. chunk size of the rx queue entries (this is what this series calls rx-buf-len), mostly influenced by MTU?

For devmem (and same for iou?), we want an option to derive (2) from (1):
page pools with larger chunks need to generate larger rx entries.

Quoted 53 lines · Stanislav Fomichev

Quoted 44 lines · Pavel Begunkov

Quoted 43 lines · Stanislav Fomichev

Quoted 41 lines · Pavel Begunkov

Quoted 29 lines · Stanislav Fomichev

Quoted 28 lines · Pavel Begunkov

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

Quoted 7 lines

Quoted 5 lines

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Don't you already need a bunch of netlink to setup rss and flow

Could be needed, but there are cases where configuration and virtual queue selection is done outside the program. I'll need to ask which option we currently use.

Quoted 6 lines

Quoted 1 line

If the setup is done outside, you can also setup rx-buf-len outside, no?

You can't do it without assuming the memory layout, and that's the application's role to allocate buffers. Not to mention that often the app won't know about all specifics either and it'd be resolved on zcrx registration.

Quoted 8 lines

I think, fundamentally, we need to distinguish:

1. chunk size of the memory pool (page pool order, niov size)
2. chunk size of the rx queue entries (this is what this series calls rx-buf-len), mostly influenced by MTU?

For devmem (and same for iou?), we want an option to derive (2) from (1):
page pools with larger chunks need to generate larger rx entries.

To be honest I'm not following. #1 and #2 seem the same to me. rx-buf-len is just the size of each rx buffer posted to the NIC.

With pp_params.order = 0 (most common configuration today), rx-buf-len == 4K. Regardless of MTU. With pp_params.order=1, I'm guessing 8K then, again regardless of MTU.

I think if the user has not configured rx-buf-len, the driver is probably free to pick whatever it wants and that can be a derivative of the MTU.

When the rx-buf-len is configured by the user, I assume the driver puts aside all MTU-related heuristics (if it has them) and uses whatever the userspace specified.

Note that the memory provider may reject the request. For example iouring and pages providers can only do page-order allocations. Devmem can in theory do any byte-aligned allocation, since gen_pool doesn't have a restriction AFAIR.

Quoted 63 lines · Mina Almasry

Quoted 53 lines · Stanislav Fomichev

Quoted 44 lines · Pavel Begunkov

Quoted 43 lines · Stanislav Fomichev

Quoted 41 lines · Pavel Begunkov

Quoted 29 lines · Stanislav Fomichev

Quoted 28 lines · Pavel Begunkov

Quoted 2 lines · Stanislav Fomichev

Quoted 1 line · Pavel Begunkov

On 7/28/25 18:13, Stanislav Fomichev wrote:

...>>> Supporting big buffers is the right direction, but I have the same

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

feedback:

Let me actually check the feedback for the queue config RFC...

Quoted 8 lines

Quoted 5 lines

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic, devmem can absolutely reuse that. I don't think there are any issues wiring up devmem?

Right, but the patch number 2 exposes per-queue rx-buf-len which I'm not sure is the right fit for devmem, see below. If all you

I guess you're talking about uapi setting it, because as an internal per queue parameter IMHO it does make sense for devmem.

Quoted 1 line

care is exposing it via io_uring, maybe don't expose it from netlink for

Sure, I can remove the set operation.

Quoted 3 lines

now? Although I'm not sure I understand why you're also passing this per-queue value via io_uring. Can you not inherit it from the queue config?

Quoted 7 lines

Quoted 5 lines

It's not a great option. It complicates user space with netlink. And there are convenience configuration features in the future that requires io_uring to parse memory first. E.g. instead of user specifying a particular size, it can say "choose the largest length under 32K that the backing memory allows".

Don't you already need a bunch of netlink to setup rss and flow

Could be needed, but there are cases where configuration and virtual queue selection is done outside the program. I'll need to ask which option we currently use.

Quoted 6 lines

Quoted 1 line

If the setup is done outside, you can also setup rx-buf-len outside, no?

You can't do it without assuming the memory layout, and that's the application's role to allocate buffers. Not to mention that often the app won't know about all specifics either and it'd be resolved on zcrx registration.

Quoted 8 lines

I think, fundamentally, we need to distinguish:

1. chunk size of the memory pool (page pool order, niov size)
2. chunk size of the rx queue entries (this is what this series calls rx-buf-len), mostly influenced by MTU?

For devmem (and same for iou?), we want an option to derive (2) from (1):
page pools with larger chunks need to generate larger rx entries.

Quoted 6 lines

To be honest I'm not following. #1 and #2 seem the same to me. rx-buf-len is just the size of each rx buffer posted to the NIC.

With pp_params.order = 0 (most common configuration today), rx-buf-len == 4K. Regardless of MTU. With pp_params.order=1, I'm guessing 8K then, again regardless of MTU.

There are drivers that fragment the buffer they get from a page pool and give smaller chunks to the hw. It's surely a good idea to be more explicit on what's what, but from the whole setup and uapi perspective I'm not too concerned.