[PATCH net-next v2 0/7] net: ethtool: fixes for HDS threshold

[PATCH net-next v2 0/7] net: ethtool: fixes for HDS threshold

[Jakub Kicinski]

Quick follow up on the HDS threshold work, since the merge window
is upon us.

Fix the bnxt implementation to apply the settings right away,
because we update the parameters _after_ configuring HW user
needed to reconfig the device twice to get the settings to stick.

For this I took the liberty of moving the config to a separate
struct. This follows my original thinking for the queue API.
It should also fit more neatly into how many drivers which
support safe config update operate. Drivers can allocate
new objects using the "pending" struct.

netdevsim:

  KTAP version 1
  1..7
  ok 1 hds.get_hds
  ok 2 hds.get_hds_thresh
  ok 3 hds.set_hds_disable
  ok 4 hds.set_hds_enable
  ok 5 hds.set_hds_thresh_zero
  ok 6 hds.set_hds_thresh_max
  ok 7 hds.set_hds_thresh_gt
  # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0

bnxt:

  KTAP version 1
  1..7
  ok 1 hds.get_hds
  ok 2 hds.get_hds_thresh
  ok 3 hds.set_hds_disable # SKIP disabling of HDS not supported by the device
  ok 4 hds.set_hds_enable
  ok 5 hds.set_hds_thresh_zero
  ok 6 hds.set_hds_thresh_max
  ok 7 hds.set_hds_thresh_gt
  # Totals: pass:6 fail:0 xfail:0 xpass:0 skip:1 error:0

v2:
 - make sure we always manipulate cfg_pending under rtnl_lock
 - split patch 2 into 2+3 for ease of review
v1: https://lore.kernel.org/20250117194815.1514410-1-kuba@kernel.org

Jakub Kicinski (7):
  net: move HDS config from ethtool state
  net: ethtool: store netdev in a temp variable in
    ethnl_default_set_doit()
  net: provide pending ring configuration in net_device
  eth: bnxt: apply hds_thrs settings correctly
  net: ethtool: populate the default HDS params in the core
  eth: bnxt: allocate enough buffer space to meet HDS threshold
  eth: bnxt: update header sizing defaults

 include/linux/ethtool.h                       |  4 ---
 include/linux/netdevice.h                     |  9 ++++++
 include/net/netdev_queues.h                   | 10 +++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 16 ++++++----
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  4 +--
 drivers/net/netdevsim/ethtool.c               |  6 +---
 drivers/net/netdevsim/netdev.c                | 10 +++----
 net/core/dev.c                                | 12 ++++++--
 net/core/devmem.c                             |  4 +--
 net/ethtool/netlink.c                         | 30 +++++++++++++++----
 net/ethtool/rings.c                           | 16 ++++++----
 11 files changed, 84 insertions(+), 37 deletions(-)

-- 
2.48.1

[PATCH net-next v2 1/7] net: move HDS config from ethtool state

[Jakub Kicinski]

Separate the HDS config from the ethtool state struct.
The HDS config contains just simple parameters, not state.
Having it as a separate struct will make it easier to clone / copy
and also long term potentially make it per-queue.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/ethtool.h                           |  4 ----
 include/linux/netdevice.h                         |  3 +++
 include/net/netdev_queues.h                       | 10 ++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  4 ++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  5 +++--
 drivers/net/netdevsim/ethtool.c                   |  9 +++++----
 drivers/net/netdevsim/netdev.c                    | 10 +++++-----
 net/core/dev.c                                    | 10 ++++++++--
 net/core/devmem.c                                 |  4 ++--
 net/ethtool/rings.c                               |  8 +++++---
 10 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 64301ddf2f59..870994cc3ef7 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1171,16 +1171,12 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
  * @rss_ctx:		XArray of custom RSS contexts
  * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
  *			within RTNL.
- * @hds_thresh:		HDS Threshold value.
- * @hds_config:		HDS value from userspace.
  * @wol_enabled:	Wake-on-LAN is enabled
  * @module_fw_flash_in_progress: Module firmware flashing is in progress.
  */
 struct ethtool_netdev_state {
 	struct xarray		rss_ctx;
 	struct mutex		rss_lock;
-	u32			hds_thresh;
-	u8			hds_config;
 	unsigned		wol_enabled:1;
 	unsigned		module_fw_flash_in_progress:1;
 };
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8308d9c75918..173a8b3a9eb2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -63,6 +63,7 @@ struct dsa_port;
 struct ip_tunnel_parm_kern;
 struct macsec_context;
 struct macsec_ops;
+struct netdev_config;
 struct netdev_name_node;
 struct sd_flow_limit;
 struct sfp_bus;
@@ -2410,6 +2411,8 @@ struct net_device {
 	const struct udp_tunnel_nic_info	*udp_tunnel_nic_info;
 	struct udp_tunnel_nic	*udp_tunnel_nic;
 
+	/** @cfg: net_device queue-related configuration */
+	struct netdev_config	*cfg;
 	struct ethtool_netdev_state *ethtool;
 
 	/* protected by rtnl_lock */
diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
index 5ca019d294ca..b02bb9f109d5 100644
--- a/include/net/netdev_queues.h
+++ b/include/net/netdev_queues.h
@@ -4,6 +4,16 @@
 
 #include <linux/netdevice.h>
 
+/**
+ * struct netdev_config - queue-related configuration for a netdev
+ * @hds_thresh:		HDS Threshold value.
+ * @hds_config:		HDS value from userspace.
+ */
+struct netdev_config {
+	u32	hds_thresh;
+	u8	hds_config;
+};
+
 /* See the netdev.yaml spec for definition of each statistic */
 struct netdev_queue_stats_rx {
 	u64 bytes;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 748c9b1ea701..0998b20578b4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4610,7 +4610,7 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 static void bnxt_init_ring_params(struct bnxt *bp)
 {
 	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
-	bp->dev->ethtool->hds_thresh = BNXT_DEFAULT_RX_COPYBREAK;
+	bp->dev->cfg->hds_thresh = BNXT_DEFAULT_RX_COPYBREAK;
 }
 
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
@@ -6585,7 +6585,7 @@ static void bnxt_hwrm_update_rss_hash_cfg(struct bnxt *bp)
 
 static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
 {
-	u16 hds_thresh = (u16)bp->dev->ethtool->hds_thresh;
+	u16 hds_thresh = (u16)bp->dev->cfg->hds_thresh;
 	struct hwrm_vnic_plcmodes_cfg_input *req;
 	int rc;
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 65a20931c579..0a6d47d4d66b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -24,6 +24,7 @@
 #include <linux/ptp_clock_kernel.h>
 #include <linux/net_tstamp.h>
 #include <linux/timecounter.h>
+#include <net/netdev_queues.h>
 #include <net/netlink.h>
 #include "bnxt_hsi.h"
 #include "bnxt.h"
@@ -834,7 +835,7 @@ static void bnxt_get_ringparam(struct net_device *dev,
 	ering->rx_jumbo_pending = bp->rx_agg_ring_size;
 	ering->tx_pending = bp->tx_ring_size;
 
-	kernel_ering->hds_thresh = dev->ethtool->hds_thresh;
+	kernel_ering->hds_thresh = dev->cfg->hds_thresh;
 	kernel_ering->hds_thresh_max = BNXT_HDS_THRESHOLD_MAX;
 }
 
@@ -852,7 +853,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
 	    (ering->tx_pending < BNXT_MIN_TX_DESC_CNT))
 		return -EINVAL;
 
-	hds_config_mod = tcp_data_split != dev->ethtool->hds_config;
+	hds_config_mod = tcp_data_split != dev->cfg->hds_config;
 	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && hds_config_mod)
 		return -EINVAL;
 
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 12163635b759..189793debdb7 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -3,6 +3,7 @@
 
 #include <linux/debugfs.h>
 #include <linux/random.h>
+#include <net/netdev_queues.h>
 
 #include "netdevsim.h"
 
@@ -71,8 +72,8 @@ static void nsim_get_ringparam(struct net_device *dev,
 	struct netdevsim *ns = netdev_priv(dev);
 
 	memcpy(ring, &ns->ethtool.ring, sizeof(ns->ethtool.ring));
-	kernel_ring->tcp_data_split = dev->ethtool->hds_config;
-	kernel_ring->hds_thresh = dev->ethtool->hds_thresh;
+	kernel_ring->tcp_data_split = dev->cfg->hds_config;
+	kernel_ring->hds_thresh = dev->cfg->hds_thresh;
 	kernel_ring->hds_thresh_max = NSIM_HDS_THRESHOLD_MAX;
 
 	if (kernel_ring->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
@@ -190,8 +191,8 @@ static void nsim_ethtool_ring_init(struct netdevsim *ns)
 	ns->ethtool.ring.rx_mini_max_pending = 4096;
 	ns->ethtool.ring.tx_max_pending = 4096;
 
-	ns->netdev->ethtool->hds_config = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
-	ns->netdev->ethtool->hds_thresh = 0;
+	ns->netdev->cfg->hds_config = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
+	ns->netdev->cfg->hds_thresh = 0;
 }
 
 void nsim_ethtool_init(struct netdevsim *ns)
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index f92b05ccdca9..42f247cbdcee 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -55,10 +55,10 @@ static int nsim_forward_skb(struct net_device *dev, struct sk_buff *skb,
 static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
-	struct ethtool_netdev_state *ethtool;
 	struct net_device *peer_dev;
 	unsigned int len = skb->len;
 	struct netdevsim *peer_ns;
+	struct netdev_config *cfg;
 	struct nsim_rq *rq;
 	int rxq;
 
@@ -76,11 +76,11 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		rxq = rxq % peer_dev->num_rx_queues;
 	rq = peer_ns->rq[rxq];
 
-	ethtool = peer_dev->ethtool;
+	cfg = peer_dev->cfg;
 	if (skb_is_nonlinear(skb) &&
-	    (ethtool->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED ||
-	     (ethtool->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
-	      ethtool->hds_thresh > len)))
+	    (cfg->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED ||
+	     (cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	      cfg->hds_thresh > len)))
 		skb_linearize(skb);
 
 	skb_tx_timestamp(skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index d7cbe6ff5249..71b3f786a9cd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -106,6 +106,7 @@
 #include <net/dst.h>
 #include <net/dst_metadata.h>
 #include <net/gro.h>
+#include <net/netdev_queues.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
 #include <net/checksum.h>
@@ -9716,7 +9717,7 @@ int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 	if (!dev->netdev_ops->ndo_bpf)
 		return -EOPNOTSUPP;
 
-	if (dev->ethtool->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
 	    bpf->command == XDP_SETUP_PROG &&
 	    bpf->prog && !bpf->prog->aux->xdp_has_frags) {
 		NL_SET_ERR_MSG(bpf->extack,
@@ -9761,7 +9762,7 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 	struct netdev_bpf xdp;
 	int err;
 
-	if (dev->ethtool->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
+	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
 	    prog && !prog->aux->xdp_has_frags) {
 		NL_SET_ERR_MSG(extack, "unable to install XDP to device using tcp-data-split");
 		return -EBUSY;
@@ -11539,6 +11540,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	if (!dev->ethtool)
 		goto free_all;
 
+	dev->cfg = kzalloc(sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
+	if (!dev->cfg)
+		goto free_all;
+
 	napi_config_sz = array_size(maxqs, sizeof(*dev->napi_config));
 	dev->napi_config = kvzalloc(napi_config_sz, GFP_KERNEL_ACCOUNT);
 	if (!dev->napi_config)
@@ -11607,6 +11612,7 @@ void free_netdev(struct net_device *dev)
 		return;
 	}
 
+	kfree(dev->cfg);
 	kfree(dev->ethtool);
 	netif_free_tx_queues(dev);
 	netif_free_rx_queues(dev);
diff --git a/net/core/devmem.c b/net/core/devmem.c
index c971b8aceac8..3bba3f018df0 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -141,12 +141,12 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 		return -ERANGE;
 	}
 
-	if (dev->ethtool->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
+	if (dev->cfg->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
 		NL_SET_ERR_MSG(extack, "tcp-data-split is disabled");
 		return -EINVAL;
 	}
 
-	if (dev->ethtool->hds_thresh) {
+	if (dev->cfg->hds_thresh) {
 		NL_SET_ERR_MSG(extack, "hds-thresh is not zero");
 		return -EINVAL;
 	}
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index d8cd4e4d7762..7a3c2a2dff12 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <net/netdev_queues.h>
+
 #include "netlink.h"
 #include "common.h"
 
@@ -219,7 +221,7 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	dev->ethtool_ops->get_ringparam(dev, &ringparam,
 					&kernel_ringparam, info->extack);
-	kernel_ringparam.tcp_data_split = dev->ethtool->hds_config;
+	kernel_ringparam.tcp_data_split = dev->cfg->hds_config;
 
 	ethnl_update_u32(&ringparam.rx_pending, tb[ETHTOOL_A_RINGS_RX], &mod);
 	ethnl_update_u32(&ringparam.rx_mini_pending,
@@ -295,8 +297,8 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, info->extack);
 	if (!ret) {
-		dev->ethtool->hds_config = kernel_ringparam.tcp_data_split;
-		dev->ethtool->hds_thresh = kernel_ringparam.hds_thresh;
+		dev->cfg->hds_config = kernel_ringparam.tcp_data_split;
+		dev->cfg->hds_thresh = kernel_ringparam.hds_thresh;
 	}
 
 	return ret < 0 ? ret : 1;
-- 
2.48.1

[PATCH net-next v2 2/7] net: ethtool: store netdev in a temp variable in ethnl_default_set_doit()

[Jakub Kicinski]

For ease of review of the next patch store the dev pointer
on the stack, instead of referring to req_info.dev every time.

No functional changes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/netlink.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 849c98e637c6..c17d8513d4c1 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -667,6 +667,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	const struct ethnl_request_ops *ops;
 	struct ethnl_req_info req_info = {};
 	const u8 cmd = info->genlhdr->cmd;
+	struct net_device *dev;
 	int ret;
 
 	ops = ethnl_default_requests[cmd];
@@ -688,19 +689,21 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 			goto out_dev;
 	}
 
+	dev = req_info.dev;
+
 	rtnl_lock();
-	ret = ethnl_ops_begin(req_info.dev);
+	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		goto out_rtnl;
 
 	ret = ops->set(&req_info, info);
 	if (ret <= 0)
 		goto out_ops;
-	ethtool_notify(req_info.dev, ops->set_ntf_cmd, NULL);
+	ethtool_notify(dev, ops->set_ntf_cmd, NULL);
 
 	ret = 0;
 out_ops:
-	ethnl_ops_complete(req_info.dev);
+	ethnl_ops_complete(dev);
 out_rtnl:
 	rtnl_unlock();
 out_dev:
-- 
2.48.1

[PATCH net-next v2 3/7] net: provide pending ring configuration in net_device

[Jakub Kicinski]

Record the pending configuration in net_device struct.
ethtool core duplicates the current config and the specific
handlers (for now just ringparam) can modify it.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - make sure all dev->cfg changes are under rtnl_lock
---
 include/linux/netdevice.h |  6 ++++++
 net/core/dev.c            |  2 ++
 net/ethtool/netlink.c     | 21 ++++++++++++++++++---
 net/ethtool/rings.c       |  8 +++-----
 4 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 173a8b3a9eb2..8da4c61f97b9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2413,6 +2413,12 @@ struct net_device {
 
 	/** @cfg: net_device queue-related configuration */
 	struct netdev_config	*cfg;
+	/**
+	 * @cfg_pending: same as @cfg but when device is being actively
+	 *	reconfigured includes any changes to the configuration
+	 *	requested by the user, but which may or may not be rejected.
+	 */
+	struct netdev_config	*cfg_pending;
 	struct ethtool_netdev_state *ethtool;
 
 	/* protected by rtnl_lock */
diff --git a/net/core/dev.c b/net/core/dev.c
index 71b3f786a9cd..a7883f1c94a0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11543,6 +11543,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	dev->cfg = kzalloc(sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
 	if (!dev->cfg)
 		goto free_all;
+	dev->cfg_pending = dev->cfg;
 
 	napi_config_sz = array_size(maxqs, sizeof(*dev->napi_config));
 	dev->napi_config = kvzalloc(napi_config_sz, GFP_KERNEL_ACCOUNT);
@@ -11612,6 +11613,7 @@ void free_netdev(struct net_device *dev)
 		return;
 	}
 
+	WARN_ON(dev->cfg != dev->cfg_pending);
 	kfree(dev->cfg);
 	kfree(dev->ethtool);
 	netif_free_tx_queues(dev);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index c17d8513d4c1..1d2f62ef6130 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
+#include <net/netdev_queues.h>
 #include <net/sock.h>
 #include <linux/ethtool_netlink.h>
 #include <linux/phy_link_topology.h>
@@ -692,19 +693,33 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	dev = req_info.dev;
 
 	rtnl_lock();
+	dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
+				   GFP_KERNEL_ACCOUNT);
+	if (!dev->cfg_pending) {
+		ret = -ENOMEM;
+		goto out_tie_cfg;
+	}
+
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
-		goto out_rtnl;
+		goto out_free_cfg;
 
 	ret = ops->set(&req_info, info);
-	if (ret <= 0)
+	if (ret < 0)
+		goto out_ops;
+
+	swap(dev->cfg, dev->cfg_pending);
+	if (!ret)
 		goto out_ops;
 	ethtool_notify(dev, ops->set_ntf_cmd, NULL);
 
 	ret = 0;
 out_ops:
 	ethnl_ops_complete(dev);
-out_rtnl:
+out_free_cfg:
+	kfree(dev->cfg_pending);
+out_tie_cfg:
+	dev->cfg_pending = dev->cfg;
 	rtnl_unlock();
 out_dev:
 	ethnl_parse_header_dev_put(&req_info);
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 7a3c2a2dff12..5e8ba81fbb3e 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -294,13 +294,11 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 		return -EINVAL;
 	}
 
+	dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
+	dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
+
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, info->extack);
-	if (!ret) {
-		dev->cfg->hds_config = kernel_ringparam.tcp_data_split;
-		dev->cfg->hds_thresh = kernel_ringparam.hds_thresh;
-	}
-
 	return ret < 0 ? ret : 1;
 }
 
-- 
2.48.1

[PATCH net-next v2 4/7] eth: bnxt: apply hds_thrs settings correctly

[Jakub Kicinski]

Use the pending config for hds_thrs. Core will only update the "current"
one after we return success. Without this change 2 reconfigs would be
required for the setting to reach the device.

Fixes: 6b43673a25c3 ("bnxt_en: add support for hds-thresh ethtool command")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 0998b20578b4..2eeed4c11b64 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6585,7 +6585,7 @@ static void bnxt_hwrm_update_rss_hash_cfg(struct bnxt *bp)
 
 static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
 {
-	u16 hds_thresh = (u16)bp->dev->cfg->hds_thresh;
+	u16 hds_thresh = (u16)bp->dev->cfg_pending->hds_thresh;
 	struct hwrm_vnic_plcmodes_cfg_input *req;
 	int rc;
 
-- 
2.48.1

[PATCH net-next v2 5/7] net: ethtool: populate the default HDS params in the core

[Jakub Kicinski]

The core has the current HDS config, it can pre-populate the values
for the drivers. While at it, remove the zero-setting in netdevsim.
Zero are the default values since the config is zalloc'ed.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 1 -
 drivers/net/netdevsim/ethtool.c                   | 5 -----
 net/ethtool/rings.c                               | 4 ++++
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 0a6d47d4d66b..9c5820839514 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -835,7 +835,6 @@ static void bnxt_get_ringparam(struct net_device *dev,
 	ering->rx_jumbo_pending = bp->rx_agg_ring_size;
 	ering->tx_pending = bp->tx_ring_size;
 
-	kernel_ering->hds_thresh = dev->cfg->hds_thresh;
 	kernel_ering->hds_thresh_max = BNXT_HDS_THRESHOLD_MAX;
 }
 
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 189793debdb7..3b23f3d3ca2b 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -72,8 +72,6 @@ static void nsim_get_ringparam(struct net_device *dev,
 	struct netdevsim *ns = netdev_priv(dev);
 
 	memcpy(ring, &ns->ethtool.ring, sizeof(ns->ethtool.ring));
-	kernel_ring->tcp_data_split = dev->cfg->hds_config;
-	kernel_ring->hds_thresh = dev->cfg->hds_thresh;
 	kernel_ring->hds_thresh_max = NSIM_HDS_THRESHOLD_MAX;
 
 	if (kernel_ring->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
@@ -190,9 +188,6 @@ static void nsim_ethtool_ring_init(struct netdevsim *ns)
 	ns->ethtool.ring.rx_jumbo_max_pending = 4096;
 	ns->ethtool.ring.rx_mini_max_pending = 4096;
 	ns->ethtool.ring.tx_max_pending = 4096;
-
-	ns->netdev->cfg->hds_config = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
-	ns->netdev->cfg->hds_thresh = 0;
 }
 
 void nsim_ethtool_init(struct netdevsim *ns)
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 5e8ba81fbb3e..7839bfd1ac6a 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -39,6 +39,10 @@ static int rings_prepare_data(const struct ethnl_req_info *req_base,
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
+
+	data->kernel_ringparam.tcp_data_split = dev->cfg->hds_config;
+	data->kernel_ringparam.hds_thresh = dev->cfg->hds_thresh;
+
 	dev->ethtool_ops->get_ringparam(dev, &data->ringparam,
 					&data->kernel_ringparam, info->extack);
 	ethnl_ops_complete(dev);
-- 
2.48.1

[PATCH net-next v2 6/7] eth: bnxt: allocate enough buffer space to meet HDS threshold

[Jakub Kicinski]

Now that we can configure HDS threshold separately from the rx_copybreak
HDS threshold may be higher than rx_copybreak.

We need to make sure that we have enough space for the headers.

Fixes: 6b43673a25c3 ("bnxt_en: add support for hds-thresh ethtool command")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 2eeed4c11b64..19e723493c4e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4671,9 +4671,10 @@ void bnxt_set_ring_params(struct bnxt *bp)
 				  ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
 				  SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 		} else {
-			rx_size = SKB_DATA_ALIGN(max(BNXT_DEFAULT_RX_COPYBREAK,
-						     bp->rx_copybreak) +
-						 NET_IP_ALIGN);
+			rx_size = max3(BNXT_DEFAULT_RX_COPYBREAK,
+				       bp->rx_copybreak,
+				       bp->dev->cfg_pending->hds_thresh);
+			rx_size = SKB_DATA_ALIGN(rx_size + NET_IP_ALIGN);
 			rx_space = rx_size + NET_SKB_PAD +
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 		}
-- 
2.48.1

[PATCH net-next v2 7/7] eth: bnxt: update header sizing defaults

[Jakub Kicinski]

300-400B RPC requests are fairly common. With the current default
of 256B HDS threshold bnxt ends up splitting those, lowering PCIe
bandwidth efficiency and increasing the number of memory allocation.

Increase the HDS threshold to fit 4 buffers in a 4k page.
This works out to 640B as the threshold on a typical kernel confing.
This change increases the performance for a microbenchmark which
receives 400B RPCs and sends empty responses by 4.5%.
Admittedly this is just a single benchmark, but 256B works out to
just 6 (so 2 more) packets per head page, because shinfo size
dominates the headers.

Now that we use page pool for the header pages I was also tempted
to default rx_copybreak to 0, but in synthetic testing the copybreak
size doesn't seem to make much difference.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 19e723493c4e..589a1008601c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4609,8 +4609,13 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 
 static void bnxt_init_ring_params(struct bnxt *bp)
 {
+	unsigned int rx_size;
+
 	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
-	bp->dev->cfg->hds_thresh = BNXT_DEFAULT_RX_COPYBREAK;
+	/* Try to fit 4 chunks into a 4k page */
+	rx_size = SZ_1K -
+		NET_SKB_PAD - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	bp->dev->cfg->hds_thresh = max(BNXT_DEFAULT_RX_COPYBREAK, rx_size);
 }
 
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
-- 
2.48.1

Re: [PATCH net-next v2 0/7] net: ethtool: fixes for HDS threshold

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On Sat, Jan 18, 2025 at 6:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Quick follow up on the HDS threshold work, since the merge window
> is upon us.
>
> Fix the bnxt implementation to apply the settings right away,
> because we update the parameters _after_ configuring HW user
> needed to reconfig the device twice to get the settings to stick.
>
> For this I took the liberty of moving the config to a separate
> struct. This follows my original thinking for the queue API.
> It should also fit more neatly into how many drivers which
> support safe config update operate. Drivers can allocate
> new objects using the "pending" struct.
>
> netdevsim:
>
>   KTAP version 1
>   1..7
>   ok 1 hds.get_hds
>   ok 2 hds.get_hds_thresh
>   ok 3 hds.set_hds_disable
>   ok 4 hds.set_hds_enable
>   ok 5 hds.set_hds_thresh_zero
>   ok 6 hds.set_hds_thresh_max
>   ok 7 hds.set_hds_thresh_gt
>   # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> bnxt:
>
>   KTAP version 1
>   1..7
>   ok 1 hds.get_hds
>   ok 2 hds.get_hds_thresh
>   ok 3 hds.set_hds_disable # SKIP disabling of HDS not supported by the device
>   ok 4 hds.set_hds_enable
>   ok 5 hds.set_hds_thresh_zero
>   ok 6 hds.set_hds_thresh_max
>   ok 7 hds.set_hds_thresh_gt
>   # Totals: pass:6 fail:0 xfail:0 xpass:0 skip:1 error:0
>
> v2:
>  - make sure we always manipulate cfg_pending under rtnl_lock
>  - split patch 2 into 2+3 for ease of review
> v1: https://lore.kernel.org/20250117194815.1514410-1-kuba@kernel.org

For the series,
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next v2 0/7] net: ethtool: fixes for HDS threshold

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 18 Jan 2025 18:05:10 -0800 you wrote:
> Quick follow up on the HDS threshold work, since the merge window
> is upon us.
> 
> Fix the bnxt implementation to apply the settings right away,
> because we update the parameters _after_ configuring HW user
> needed to reconfig the device twice to get the settings to stick.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/7] net: move HDS config from ethtool state
    (no matching commit)
  - [net-next,v2,2/7] net: ethtool: store netdev in a temp variable in ethnl_default_set_doit()
    https://git.kernel.org/netdev/net-next/c/743dea746ed6
  - [net-next,v2,3/7] net: provide pending ring configuration in net_device
    https://git.kernel.org/netdev/net-next/c/32ad1f7a050d
  - [net-next,v2,4/7] eth: bnxt: apply hds_thrs settings correctly
    https://git.kernel.org/netdev/net-next/c/e58263e91117
  - [net-next,v2,5/7] net: ethtool: populate the default HDS params in the core
    https://git.kernel.org/netdev/net-next/c/928459bbda19
  - [net-next,v2,6/7] eth: bnxt: allocate enough buffer space to meet HDS threshold
    https://git.kernel.org/netdev/net-next/c/bee018052d1b
  - [net-next,v2,7/7] eth: bnxt: update header sizing defaults
    https://git.kernel.org/netdev/net-next/c/99d028c63457

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v2 5/7] net: ethtool: populate the default HDS params in the core

[Eric Dumazet]

On Sun, Jan 19, 2025 at 3:05 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> The core has the current HDS config, it can pre-populate the values
> for the drivers. While at it, remove the zero-setting in netdevsim.
> Zero are the default values since the config is zalloc'ed.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 1 -
>  drivers/net/netdevsim/ethtool.c                   | 5 -----
>  net/ethtool/rings.c                               | 4 ++++
>  3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 0a6d47d4d66b..9c5820839514 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -835,7 +835,6 @@ static void bnxt_get_ringparam(struct net_device *dev,
>         ering->rx_jumbo_pending = bp->rx_agg_ring_size;
>         ering->tx_pending = bp->tx_ring_size;
>
> -       kernel_ering->hds_thresh = dev->cfg->hds_thresh;
>         kernel_ering->hds_thresh_max = BNXT_HDS_THRESHOLD_MAX;
>  }
>
> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
> index 189793debdb7..3b23f3d3ca2b 100644
> --- a/drivers/net/netdevsim/ethtool.c
> +++ b/drivers/net/netdevsim/ethtool.c
> @@ -72,8 +72,6 @@ static void nsim_get_ringparam(struct net_device *dev,
>         struct netdevsim *ns = netdev_priv(dev);
>
>         memcpy(ring, &ns->ethtool.ring, sizeof(ns->ethtool.ring));
> -       kernel_ring->tcp_data_split = dev->cfg->hds_config;
> -       kernel_ring->hds_thresh = dev->cfg->hds_thresh;
>         kernel_ring->hds_thresh_max = NSIM_HDS_THRESHOLD_MAX;
>
>         if (kernel_ring->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
> @@ -190,9 +188,6 @@ static void nsim_ethtool_ring_init(struct netdevsim *ns)
>         ns->ethtool.ring.rx_jumbo_max_pending = 4096;
>         ns->ethtool.ring.rx_mini_max_pending = 4096;
>         ns->ethtool.ring.tx_max_pending = 4096;
> -
> -       ns->netdev->cfg->hds_config = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
> -       ns->netdev->cfg->hds_thresh = 0;
>  }
>
>  void nsim_ethtool_init(struct netdevsim *ns)
> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
> index 5e8ba81fbb3e..7839bfd1ac6a 100644
> --- a/net/ethtool/rings.c
> +++ b/net/ethtool/rings.c
> @@ -39,6 +39,10 @@ static int rings_prepare_data(const struct ethnl_req_info *req_base,
>         ret = ethnl_ops_begin(dev);
>         if (ret < 0)
>                 return ret;
> +
> +       data->kernel_ringparam.tcp_data_split = dev->cfg->hds_config;
> +       data->kernel_ringparam.hds_thresh = dev->cfg->hds_thresh;
> +
>         dev->ethtool_ops->get_ringparam(dev, &data->ringparam,
>                                         &data->kernel_ringparam, info->extack);
>         ethnl_ops_complete(dev);
> --
> 2.48.1

This patch makes syzbot unhappy [1]

I am unsure how to fix this, should all callers to
dev->ethtool_ops->get_ringparam()
have to populate  tcp_data_split and hds_thresh from dev->cfg,
or would the following fix be enough ?

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 7bb94875a7ec87b3e2d882cb5df2416b9fad9d9..70461ff5c54cb787c2047ac4d67c6b0305db2b6
100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2060,7 +2060,7 @@ static int ethtool_get_ringparam(struct
net_device *dev, void __user *useraddr)
 static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
 {
        struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM };
-       struct kernel_ethtool_ringparam kernel_ringparam;
+       struct kernel_ethtool_ringparam kernel_ringparam = {};
        int ret;

        if (!dev->ethtool_ops->set_ringparam ||
!dev->ethtool_ops->get_ringparam)

[1]
=====================================================
BUG: KMSAN: uninit-value in nsim_get_ringparam+0xa8/0xe0
drivers/net/netdevsim/ethtool.c:77
nsim_get_ringparam+0xa8/0xe0 drivers/net/netdevsim/ethtool.c:77
ethtool_set_ringparam+0x268/0x570 net/ethtool/ioctl.c:2072
__dev_ethtool net/ethtool/ioctl.c:3209 [inline]
dev_ethtool+0x126d/0x2a40 net/ethtool/ioctl.c:3398
dev_ioctl+0xb0e/0x1280 net/core/dev_ioctl.c:759
sock_do_ioctl+0x28c/0x540 net/socket.c:1208
sock_ioctl+0x721/0xd70 net/socket.c:1313
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:906 [inline]
__se_sys_ioctl+0x246/0x440 fs/ioctl.c:892
__x64_sys_ioctl+0x96/0xe0 fs/ioctl.c:892
x64_sys_call+0x19f0/0x3c30 arch/x86/include/generated/asm/syscalls_64.h:17
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f

Local variable kernel_ringparam created at:
ethtool_set_ringparam+0x96/0x570 net/ethtool/ioctl.c:2063
__dev_ethtool net/ethtool/ioctl.c:3209 [inline]
dev_ethtool+0x126d/0x2a40 net/ethtool/ioctl.c:3398

CPU: 0 UID: 0 PID: 5807 Comm: syz-executor164 Not tainted
6.13.0-syzkaller-04788-g7004a2e46d16 #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 12/27/2024

Re: [PATCH net-next v2 5/7] net: ethtool: populate the default HDS params in the core

[Jakub Kicinski]

On Thu, 23 Jan 2025 17:15:55 +0100 Eric Dumazet wrote:
> I am unsure how to fix this, should all callers to
> dev->ethtool_ops->get_ringparam()
> have to populate  tcp_data_split and hds_thresh from dev->cfg,
> or would the following fix be enough ?

I think the comparison in nsim_get_ringparam() should look at dev->cfg.
If the used configuration (dev->cfg) is not set (UNKNOWN), our default
is ENABLED.

Re: [PATCH net-next v2 5/7] net: ethtool: populate the default HDS params in the core

[Eric Dumazet]

On Thu, Jan 23, 2025 at 5:22 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 23 Jan 2025 17:15:55 +0100 Eric Dumazet wrote:
> > I am unsure how to fix this, should all callers to
> > dev->ethtool_ops->get_ringparam()
> > have to populate  tcp_data_split and hds_thresh from dev->cfg,
> > or would the following fix be enough ?
>
> I think the comparison in nsim_get_ringparam() should look at dev->cfg.
> If the used configuration (dev->cfg) is not set (UNKNOWN), our default
> is ENABLED.

Hmm, I am releasing the syzbot report and let you deal with it then.

Thanks !