[PATCH V4 net-next 0/6] ethtool: add support to set/get tx copybreak buf size and rx buf len

[PATCH V4 net-next 0/6] ethtool: add support to set/get tx copybreak buf size and rx buf len

[Guangbin Huang]

From: Hao Chen <chenhao288@hisilicon.com>

This series add support to set/get tx copybreak buf size and rx buf len via
ethtool and hns3 driver implements them.

Tx copybreak buf size is used for tx copybreak feature which for small size
packet or frag. Use ethtool --get-tunable command to get it, and ethtool
--set-tunable command to set it, examples are as follow:

1. set tx spare buf size to 102400:
$ ethtool --set-tunable eth1 tx-buf-size 102400

2. get tx spare buf size:
$ ethtool --get-tunable eth1 tx-buf-size
tx-buf-size: 102400

Rx buf len is buffer length of each rx BD. Use ethtool -g command to get
it, and ethtool -G command to set it, examples are as follow:

1. set rx buf len to 4096
$ ethtool -G eth1 rx-buf-len 4096

2. get rx buf len
$ ethtool -g eth1
...
RX Buf Len:     4096


Change log:
V3 -> V4
1.Fix a few allmodconfig compile warning.
2.Add more '=' synbol to ethtool-netlink.rst to refine format.
3.Move definement of struct ethtool_ringparam_ext to include/linux/ethtool.h.
4.Move related modify of rings_fill_reply() from patch 4/6 to patch 3/6.

V2 -> V3
1.Remove documentation for tx copybreak buf size, there is description for
it in userspace ethtool.
2.Move extending parameters for get/set_ringparam function from patch3/6
to patch 4/6.

V1 -> V2
1.Add documentation for rx buf len and tx copybreak buf size.
2.Extend structure ringparam_ext for extenal ring params.
3.Change type of ETHTOOL_A_RINGS_RX_BUF_LEN from NLA_U32 to
  NLA_POLICY_MIN(NLA_U32, 1).
4.Add supported_ring_params in ethtool_ops to indicate if support external
  params.

Hao Chen (6):
  ethtool: add support to set/get tx copybreak buf size via ethtool
  net: hns3: add support to set/get tx copybreak buf size via ethtool
    for hns3 driver
  ethtool: add support to set/get rx buf len via ethtool
  ethtool: extend ringparam setting uAPI with rx_buf_len
  net: hns3: add support to set/get rx buf len via ethtool for hns3
    driver
  net: hns3: remove the way to set tx spare buf via module parameter

 Documentation/networking/ethtool-netlink.rst  |  10 +-
 arch/um/drivers/vector_kern.c                 |   4 +-
 drivers/net/can/c_can/c_can_ethtool.c         |   4 +-
 drivers/net/ethernet/3com/typhoon.c           |   4 +-
 drivers/net/ethernet/amazon/ena/ena_ethtool.c |   8 +-
 drivers/net/ethernet/amd/pcnet32.c            |   8 +-
 drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c  |   8 +-
 .../ethernet/aquantia/atlantic/aq_ethtool.c   |   8 +-
 drivers/net/ethernet/atheros/atlx/atl1.c      |   8 +-
 drivers/net/ethernet/broadcom/b44.c           |   8 +-
 drivers/net/ethernet/broadcom/bcm63xx_enet.c  |  16 ++-
 drivers/net/ethernet/broadcom/bnx2.c          |   8 +-
 .../ethernet/broadcom/bnx2x/bnx2x_ethtool.c   |   8 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |   8 +-
 drivers/net/ethernet/broadcom/tg3.c           |  10 +-
 .../net/ethernet/brocade/bna/bnad_ethtool.c   |   8 +-
 drivers/net/ethernet/cadence/macb_main.c      |   8 +-
 .../ethernet/cavium/liquidio/lio_ethtool.c    |   8 +-
 .../ethernet/cavium/thunder/nicvf_ethtool.c   |   8 +-
 drivers/net/ethernet/chelsio/cxgb/cxgb2.c     |   8 +-
 .../net/ethernet/chelsio/cxgb3/cxgb3_main.c   |   8 +-
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    |   8 +-
 .../ethernet/chelsio/cxgb4vf/cxgb4vf_main.c   |   8 +-
 .../net/ethernet/cisco/enic/enic_ethtool.c    |   8 +-
 drivers/net/ethernet/cortina/gemini.c         |   8 +-
 .../net/ethernet/emulex/benet/be_ethtool.c    |   4 +-
 drivers/net/ethernet/ethoc.c                  |   8 +-
 drivers/net/ethernet/faraday/ftgmac100.c      |   8 +-
 .../ethernet/freescale/enetc/enetc_ethtool.c  |   4 +-
 .../net/ethernet/freescale/gianfar_ethtool.c  |   8 +-
 .../net/ethernet/freescale/ucc_geth_ethtool.c |   8 +-
 drivers/net/ethernet/google/gve/gve_ethtool.c |   4 +-
 .../net/ethernet/hisilicon/hns/hns_ethtool.c  |   6 +-
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  11 +-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   |   2 +
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    | 116 ++++++++++++++++--
 .../net/ethernet/huawei/hinic/hinic_ethtool.c |   8 +-
 drivers/net/ethernet/ibm/emac/core.c          |   4 +-
 drivers/net/ethernet/ibm/ibmvnic.c            |   8 +-
 drivers/net/ethernet/intel/e100.c             |   8 +-
 .../net/ethernet/intel/e1000/e1000_ethtool.c  |   8 +-
 drivers/net/ethernet/intel/e1000e/ethtool.c   |   8 +-
 .../net/ethernet/intel/fm10k/fm10k_ethtool.c  |   8 +-
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |   8 +-
 .../net/ethernet/intel/iavf/iavf_ethtool.c    |  12 +-
 drivers/net/ethernet/intel/ice/ice_ethtool.c  |   8 +-
 drivers/net/ethernet/intel/igb/igb_ethtool.c  |   8 +-
 drivers/net/ethernet/intel/igbvf/ethtool.c    |   8 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c  |   8 +-
 .../net/ethernet/intel/ixgb/ixgb_ethtool.c    |   8 +-
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |   8 +-
 drivers/net/ethernet/intel/ixgbevf/ethtool.c  |   8 +-
 drivers/net/ethernet/marvell/mv643xx_eth.c    |   8 +-
 drivers/net/ethernet/marvell/mvneta.c         |   8 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |   8 +-
 .../marvell/octeontx2/nic/otx2_ethtool.c      |   8 +-
 drivers/net/ethernet/marvell/skge.c           |   8 +-
 drivers/net/ethernet/marvell/sky2.c           |   8 +-
 .../net/ethernet/mellanox/mlx4/en_ethtool.c   |   8 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |   8 +-
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |   8 +-
 .../mellanox/mlx5/core/ipoib/ethtool.c        |   8 +-
 .../mellanox/mlxbf_gige/mlxbf_gige_ethtool.c  |   4 +-
 drivers/net/ethernet/micrel/ksz884x.c         |   6 +-
 .../net/ethernet/myricom/myri10ge/myri10ge.c  |   4 +-
 drivers/net/ethernet/neterion/s2io.c          |   4 +-
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  |   8 +-
 drivers/net/ethernet/nvidia/forcedeth.c       |  10 +-
 .../oki-semi/pch_gbe/pch_gbe_ethtool.c        |  12 +-
 .../net/ethernet/pasemi/pasemi_mac_ethtool.c  |   4 +-
 .../ethernet/pensando/ionic/ionic_ethtool.c   |   8 +-
 .../qlogic/netxen/netxen_nic_ethtool.c        |   8 +-
 .../net/ethernet/qlogic/qede/qede_ethtool.c   |   8 +-
 .../ethernet/qlogic/qlcnic/qlcnic_ethtool.c   |   8 +-
 .../net/ethernet/qualcomm/emac/emac-ethtool.c |   8 +-
 drivers/net/ethernet/qualcomm/qca_debug.c     |   8 +-
 drivers/net/ethernet/realtek/8139cp.c         |   4 +-
 drivers/net/ethernet/realtek/r8169_main.c     |   4 +-
 drivers/net/ethernet/renesas/ravb_main.c      |   8 +-
 drivers/net/ethernet/renesas/sh_eth.c         |   8 +-
 drivers/net/ethernet/sfc/ef100_ethtool.c      |   4 +-
 drivers/net/ethernet/sfc/ethtool.c            |   8 +-
 drivers/net/ethernet/sfc/falcon/ethtool.c     |   8 +-
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |   8 +-
 drivers/net/ethernet/tehuti/tehuti.c          |  10 +-
 drivers/net/ethernet/ti/am65-cpsw-ethtool.c   |   4 +-
 drivers/net/ethernet/ti/cpmac.c               |   8 +-
 drivers/net/ethernet/ti/cpsw_ethtool.c        |   8 +-
 drivers/net/ethernet/ti/cpsw_priv.h           |   8 +-
 .../net/ethernet/toshiba/spider_net_ethtool.c |   4 +-
 drivers/net/ethernet/xilinx/ll_temac_main.c   |  14 ++-
 .../net/ethernet/xilinx/xilinx_axienet_main.c |  14 ++-
 drivers/net/hyperv/netvsc_drv.c               |   8 +-
 drivers/net/netdevsim/ethtool.c               |   8 +-
 drivers/net/usb/r8152.c                       |   8 +-
 drivers/net/virtio_net.c                      |   4 +-
 drivers/net/vmxnet3/vmxnet3_ethtool.c         |   8 +-
 drivers/s390/net/qeth_ethtool.c               |   4 +-
 include/linux/ethtool.h                       |  26 +++-
 include/uapi/linux/ethtool.h                  |   1 +
 include/uapi/linux/ethtool_netlink.h          |   1 +
 net/ethtool/common.c                          |   1 +
 net/ethtool/ioctl.c                           |  10 +-
 net/ethtool/netlink.h                         |   2 +-
 net/ethtool/rings.c                           |  32 ++++-
 net/mac80211/ethtool.c                        |   8 +-
 106 files changed, 721 insertions(+), 209 deletions(-)

-- 
2.33.0

[PATCH V4 net-next 1/6] ethtool: add support to set/get tx copybreak buf size via ethtool

[Guangbin Huang]

From: Hao Chen <chenhao288@hisilicon.com>

Add support for ethtool to set/get tx copybreak buf size.

Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 include/uapi/linux/ethtool.h | 1 +
 net/ethtool/common.c         | 1 +
 net/ethtool/ioctl.c          | 1 +
 3 files changed, 3 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index a2223b685451..7bc4b8def12c 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -231,6 +231,7 @@ enum tunable_id {
 	ETHTOOL_RX_COPYBREAK,
 	ETHTOOL_TX_COPYBREAK,
 	ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
+	ETHTOOL_TX_COPYBREAK_BUF_SIZE,
 	/*
 	 * Add your fresh new tunable attribute above and remember to update
 	 * tunable_strings[] in net/ethtool/common.c
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index c63e0739dc6a..0c5210015911 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -89,6 +89,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_RX_COPYBREAK]	= "rx-copybreak",
 	[ETHTOOL_TX_COPYBREAK]	= "tx-copybreak",
 	[ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
+	[ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
 };
 
 const char
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index bf6e8c2f9bf7..617ebc4183d9 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2383,6 +2383,7 @@ static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
 	switch (tuna->id) {
 	case ETHTOOL_RX_COPYBREAK:
 	case ETHTOOL_TX_COPYBREAK:
+	case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
 		if (tuna->len != sizeof(u32) ||
 		    tuna->type_id != ETHTOOL_TUNABLE_U32)
 			return -EINVAL;
-- 
2.33.0

[PATCH V4 net-next 2/6] net: hns3: add support to set/get tx copybreak buf size via ethtool for hns3 driver

[Guangbin Huang]

From: Hao Chen <chenhao288@hisilicon.com>

Tx copybreak buf size is used for tx copybreak feature, the feature is
used for small size packet or frag. It adds a queue based tx shared
bounce buffer to memcpy the small packet when the len of xmitted skb is
below tx_copybreak(value to distinguish small size and normal size),
and reduce the overhead of dma map and unmap when IOMMU is on.

Support setting it via ethtool --set-tunable parameter and getting
it via ethtool --get-tunable parameter.

Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  4 +-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   |  2 +
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    | 56 +++++++++++++++++++
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index fea1be4c02ed..0dd8987d1228 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5517,8 +5517,8 @@ static int hns3_reset_notify_uninit_enet(struct hnae3_handle *handle)
 	return 0;
 }
 
-static int hns3_reset_notify(struct hnae3_handle *handle,
-			     enum hnae3_reset_notify_type type)
+int hns3_reset_notify(struct hnae3_handle *handle,
+		      enum hnae3_reset_notify_type type)
 {
 	int ret = 0;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index 6162d9f88e37..eefdcef341d2 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
@@ -705,6 +705,8 @@ void hns3_set_vector_coalesce_tx_ql(struct hns3_enet_tqp_vector *tqp_vector,
 				    u32 ql_value);
 
 void hns3_request_update_promisc_mode(struct hnae3_handle *handle);
+int hns3_reset_notify(struct hnae3_handle *handle,
+		      enum hnae3_reset_notify_type type);
 
 #ifdef CONFIG_HNS3_DCB
 void hns3_dcbnl_setup(struct hnae3_handle *handle);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 5ebd96f6833d..3b74b4824f1d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -1693,6 +1693,7 @@ static int hns3_get_tunable(struct net_device *netdev,
 			    void *data)
 {
 	struct hns3_nic_priv *priv = netdev_priv(netdev);
+	struct hnae3_handle *h = priv->ae_handle;
 	int ret = 0;
 
 	switch (tuna->id) {
@@ -1703,6 +1704,9 @@ static int hns3_get_tunable(struct net_device *netdev,
 	case ETHTOOL_RX_COPYBREAK:
 		*(u32 *)data = priv->rx_copybreak;
 		break;
+	case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
+		*(u32 *)data = h->kinfo.tx_spare_buf_size;
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -1711,11 +1715,43 @@ static int hns3_get_tunable(struct net_device *netdev,
 	return ret;
 }
 
+static int hns3_set_tx_spare_buf_size(struct net_device *netdev,
+				      u32 data)
+{
+	struct hns3_nic_priv *priv = netdev_priv(netdev);
+	struct hnae3_handle *h = priv->ae_handle;
+	int ret;
+
+	if (hns3_nic_resetting(netdev))
+		return -EBUSY;
+
+	h->kinfo.tx_spare_buf_size = data;
+
+	ret = hns3_reset_notify(h, HNAE3_DOWN_CLIENT);
+	if (ret)
+		return ret;
+
+	ret = hns3_reset_notify(h, HNAE3_UNINIT_CLIENT);
+	if (ret)
+		return ret;
+
+	ret = hns3_reset_notify(h, HNAE3_INIT_CLIENT);
+	if (ret)
+		return ret;
+
+	ret = hns3_reset_notify(h, HNAE3_UP_CLIENT);
+	if (ret)
+		hns3_reset_notify(h, HNAE3_UNINIT_CLIENT);
+
+	return ret;
+}
+
 static int hns3_set_tunable(struct net_device *netdev,
 			    const struct ethtool_tunable *tuna,
 			    const void *data)
 {
 	struct hns3_nic_priv *priv = netdev_priv(netdev);
+	u32 old_tx_spare_buf_size, new_tx_spare_buf_size;
 	struct hnae3_handle *h = priv->ae_handle;
 	int i, ret = 0;
 
@@ -1733,6 +1769,26 @@ static int hns3_set_tunable(struct net_device *netdev,
 		for (i = h->kinfo.num_tqps; i < h->kinfo.num_tqps * 2; i++)
 			priv->ring[i].rx_copybreak = priv->rx_copybreak;
 
+		break;
+	case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
+		old_tx_spare_buf_size = h->kinfo.tx_spare_buf_size;
+		new_tx_spare_buf_size = *(u32 *)data;
+		ret = hns3_set_tx_spare_buf_size(netdev, new_tx_spare_buf_size);
+		if (ret) {
+			int ret1;
+
+			netdev_warn(netdev,
+				    "change tx spare buf size fail, revert to old value\n");
+			ret1 = hns3_set_tx_spare_buf_size(netdev,
+							  old_tx_spare_buf_size);
+			if (ret1) {
+				netdev_err(netdev,
+					   "revert to old tx spare buf size fail\n");
+				return ret1;
+			}
+
+			return ret;
+		}
 		break;
 	default:
 		ret = -EOPNOTSUPP;
-- 
2.33.0

[PATCH V4 net-next 3/6] ethtool: add support to set/get rx buf len via ethtool

[Guangbin Huang]

From: Hao Chen <chenhao288@hisilicon.com>

Add support to set rx buf len via ethtool -G parameter and get
rx buf len via ethtool -g parameter.

Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 Documentation/networking/ethtool-netlink.rst | 10 +++++----
 include/linux/ethtool.h                      | 18 +++++++++++++++
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.h                        |  2 +-
 net/ethtool/rings.c                          | 23 ++++++++++++++++++--
 5 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 7b598c7e3912..9d98e0511249 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -849,7 +849,7 @@ Request contents:
 
 Kernel response contents:
 
-  ====================================  ======  ==========================
+  ====================================  ======  ===========================
   ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
   ``ETHTOOL_A_RINGS_RX_MAX``            u32     max size of RX ring
   ``ETHTOOL_A_RINGS_RX_MINI_MAX``       u32     max size of RX mini ring
@@ -859,7 +859,8 @@ Kernel response contents:
   ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
   ``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
+  ====================================  ======  ===========================
 
 
 RINGS_SET
@@ -869,13 +870,14 @@ Sets ring sizes like ``ETHTOOL_SRINGPARAM`` ioctl request.
 
 Request contents:
 
-  ====================================  ======  ==========================
+  ====================================  ======  ===========================
   ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
   ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
   ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
   ``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
+  ====================================  ======  ===========================
 
 Kernel checks that requested ring sizes do not exceed limits reported by
 driver. Driver may impose additional constraints and may not suspport all
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 845a0ffc16ee..2e1111804014 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -67,6 +67,22 @@ enum {
 	ETH_RSS_HASH_FUNCS_COUNT
 };
 
+/**
+ * struct ethtool_ringparam_ext - RX/TX ring configuration
+ * @rx_buf_len: Current length of buffers on the rx ring.
+ */
+struct ethtool_ringparam_ext {
+	__u32	rx_buf_len;
+};
+
+/**
+ * enum ethtool_supported_ring_param - indicator caps for setting ring params
+ * @ETHTOOL_RING_USE_RX_BUF_LEN: capture for setting rx_buf_len
+ */
+enum ethtool_supported_ring_param {
+	ETHTOOL_RING_USE_RX_BUF_LEN = BIT(0),
+};
+
 #define __ETH_RSS_HASH_BIT(bit)	((u32)1 << (bit))
 #define __ETH_RSS_HASH(name)	__ETH_RSS_HASH_BIT(ETH_RSS_HASH_##name##_BIT)
 
@@ -432,6 +448,7 @@ struct ethtool_module_power_mode_params {
  * @cap_link_lanes_supported: indicates if the driver supports lanes
  *	parameter.
  * @supported_coalesce_params: supported types of interrupt coalescing.
+ * @supported_ring_params: supported ring params.
  * @get_drvinfo: Report driver/device information.  Should only set the
  *	@driver, @version, @fw_version and @bus_info fields.  If not
  *	implemented, the @driver and @bus_info fields will be filled in
@@ -613,6 +630,7 @@ struct ethtool_module_power_mode_params {
 struct ethtool_ops {
 	u32     cap_link_lanes_supported:1;
 	u32	supported_coalesce_params;
+	u32	supported_ring_params;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
 	int	(*get_regs_len)(struct net_device *);
 	void	(*get_regs)(struct net_device *, struct ethtool_regs *, void *);
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index ca5fbb59fa42..f31d66d636b2 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -329,6 +329,7 @@ enum {
 	ETHTOOL_A_RINGS_RX_MINI,			/* u32 */
 	ETHTOOL_A_RINGS_RX_JUMBO,			/* u32 */
 	ETHTOOL_A_RINGS_TX,				/* u32 */
+	ETHTOOL_A_RINGS_RX_BUF_LEN,                     /* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_RINGS_CNT,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 836ee7157848..490598e5eedd 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -356,7 +356,7 @@ extern const struct nla_policy ethnl_features_set_policy[ETHTOOL_A_FEATURES_WANT
 extern const struct nla_policy ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_HEADER + 1];
 extern const struct nla_policy ethnl_privflags_set_policy[ETHTOOL_A_PRIVFLAGS_FLAGS + 1];
 extern const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_HEADER + 1];
-extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_TX + 1];
+extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_RX_BUF_LEN + 1];
 extern const struct nla_policy ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_HEADER + 1];
 extern const struct nla_policy ethnl_channels_set_policy[ETHTOOL_A_CHANNELS_COMBINED_COUNT + 1];
 extern const struct nla_policy ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_HEADER + 1];
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 4e097812a967..b9ed6af34d3c 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -10,6 +10,7 @@ struct rings_req_info {
 struct rings_reply_data {
 	struct ethnl_reply_data		base;
 	struct ethtool_ringparam	ringparam;
+	struct ethtool_ringparam_ext	ringparam_ext;
 };
 
 #define RINGS_REPDATA(__reply_base) \
@@ -49,7 +50,8 @@ static int rings_reply_size(const struct ethnl_req_info *req_base,
 	       nla_total_size(sizeof(u32)) +	/* _RINGS_RX */
 	       nla_total_size(sizeof(u32)) +	/* _RINGS_RX_MINI */
 	       nla_total_size(sizeof(u32)) +	/* _RINGS_RX_JUMBO */
-	       nla_total_size(sizeof(u32));	/* _RINGS_TX */
+	       nla_total_size(sizeof(u32)) +	/* _RINGS_TX */
+	       nla_total_size(sizeof(u32));     /* _RINGS_RX_BUF_LEN */
 }
 
 static int rings_fill_reply(struct sk_buff *skb,
@@ -57,6 +59,7 @@ static int rings_fill_reply(struct sk_buff *skb,
 			    const struct ethnl_reply_data *reply_base)
 {
 	const struct rings_reply_data *data = RINGS_REPDATA(reply_base);
+	const struct ethtool_ringparam_ext *ringparam_ext = &data->ringparam_ext;
 	const struct ethtool_ringparam *ringparam = &data->ringparam;
 
 	if ((ringparam->rx_max_pending &&
@@ -78,7 +81,10 @@ static int rings_fill_reply(struct sk_buff *skb,
 	     (nla_put_u32(skb, ETHTOOL_A_RINGS_TX_MAX,
 			  ringparam->tx_max_pending) ||
 	      nla_put_u32(skb, ETHTOOL_A_RINGS_TX,
-			  ringparam->tx_pending))))
+			  ringparam->tx_pending)))  ||
+	    (ringparam_ext->rx_buf_len &&
+	     (nla_put_u32(skb, ETHTOOL_A_RINGS_RX_BUF_LEN,
+			  ringparam_ext->rx_buf_len))))
 		return -EMSGSIZE;
 
 	return 0;
@@ -105,10 +111,12 @@ 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),
 };
 
 int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
 {
+	struct ethtool_ringparam_ext ringparam_ext = {};
 	struct ethtool_ringparam ringparam = {};
 	struct ethnl_req_info req_info = {};
 	struct nlattr **tb = info->attrs;
@@ -142,6 +150,8 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
 	ethnl_update_u32(&ringparam.rx_jumbo_pending,
 			 tb[ETHTOOL_A_RINGS_RX_JUMBO], &mod);
 	ethnl_update_u32(&ringparam.tx_pending, tb[ETHTOOL_A_RINGS_TX], &mod);
+	ethnl_update_u32(&ringparam_ext.rx_buf_len,
+			 tb[ETHTOOL_A_RINGS_RX_BUF_LEN], &mod);
 	ret = 0;
 	if (!mod)
 		goto out_ops;
@@ -164,6 +174,15 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
 		goto out_ops;
 	}
 
+	if (ringparam_ext.rx_buf_len != 0 &&
+	    !(ops->supported_ring_params & ETHTOOL_RING_USE_RX_BUF_LEN)) {
+		ret = -EOPNOTSUPP;
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_RINGS_RX_BUF_LEN],
+				    "setting rx buf len not supported");
+		goto out_ops;
+	}
+
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam);
 	if (ret < 0)
 		goto out_ops;
-- 
2.33.0

[PATCH V4 net-next 5/6] net: hns3: add support to set/get rx buf len via ethtool for hns3 driver

[Guangbin Huang]

From: Hao Chen <chenhao288@hisilicon.com>

Rx buf len is for rx BD buffer size, support setting it via ethtool -G
parameter and getting it via ethtool -g parameter.

Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    | 52 ++++++++++++++++---
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index e860435298f0..f7187f602b9b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -647,7 +647,7 @@ static void hns3_get_ringparam(struct net_device *netdev,
 {
 	struct hns3_nic_priv *priv = netdev_priv(netdev);
 	struct hnae3_handle *h = priv->ae_handle;
-	int queue_num = h->kinfo.num_tqps;
+	int rx_queue_index = h->kinfo.num_tqps;
 
 	if (hns3_nic_resetting(netdev)) {
 		netdev_err(netdev, "dev resetting!");
@@ -658,7 +658,8 @@ static void hns3_get_ringparam(struct net_device *netdev,
 	param->rx_max_pending = HNS3_RING_MAX_PENDING;
 
 	param->tx_pending = priv->ring[0].desc_num;
-	param->rx_pending = priv->ring[queue_num].desc_num;
+	param->rx_pending = priv->ring[rx_queue_index].desc_num;
+	param_ext->rx_buf_len = priv->ring[rx_queue_index].buf_size;
 }
 
 static void hns3_get_pauseparam(struct net_device *netdev,
@@ -1060,14 +1061,23 @@ static struct hns3_enet_ring *hns3_backup_ringparam(struct hns3_nic_priv *priv)
 }
 
 static int hns3_check_ringparam(struct net_device *ndev,
-				struct ethtool_ringparam *param)
+				struct ethtool_ringparam *param,
+				struct ethtool_ringparam_ext *param_ext)
 {
+#define RX_BUF_LEN_2K 2048
+#define RX_BUF_LEN_4K 4096
 	if (hns3_nic_resetting(ndev))
 		return -EBUSY;
 
 	if (param->rx_mini_pending || param->rx_jumbo_pending)
 		return -EINVAL;
 
+	if (param_ext->rx_buf_len != RX_BUF_LEN_2K &&
+	    param_ext->rx_buf_len != RX_BUF_LEN_4K) {
+		netdev_err(ndev, "Rx buf len only support 2048 and 4096\n");
+		return -EINVAL;
+	}
+
 	if (param->tx_pending > HNS3_RING_MAX_PENDING ||
 	    param->tx_pending < HNS3_RING_MIN_PENDING ||
 	    param->rx_pending > HNS3_RING_MAX_PENDING ||
@@ -1080,6 +1090,22 @@ static int hns3_check_ringparam(struct net_device *ndev,
 	return 0;
 }
 
+static int hns3_change_rx_buf_len(struct net_device *ndev, u32 rx_buf_len)
+{
+	struct hns3_nic_priv *priv = netdev_priv(ndev);
+	struct hnae3_handle *h = priv->ae_handle;
+	int i;
+
+	h->kinfo.rx_buf_len = rx_buf_len;
+
+	for (i = 0; i < h->kinfo.num_tqps; i++) {
+		h->kinfo.tqp[i]->buf_size = rx_buf_len;
+		priv->ring[i + h->kinfo.num_tqps].buf_size = rx_buf_len;
+	}
+
+	return 0;
+}
+
 static int hns3_set_ringparam(struct net_device *ndev,
 			      struct ethtool_ringparam *param,
 			      struct ethtool_ringparam_ext *param_ext,
@@ -1092,9 +1118,10 @@ static int hns3_set_ringparam(struct net_device *ndev,
 	u32 old_tx_desc_num, new_tx_desc_num;
 	u32 old_rx_desc_num, new_rx_desc_num;
 	u16 queue_num = h->kinfo.num_tqps;
+	u32 old_rx_buf_len;
 	int ret, i;
 
-	ret = hns3_check_ringparam(ndev, param);
+	ret = hns3_check_ringparam(ndev, param, param_ext);
 	if (ret)
 		return ret;
 
@@ -1103,8 +1130,10 @@ static int hns3_set_ringparam(struct net_device *ndev,
 	new_rx_desc_num = ALIGN(param->rx_pending, HNS3_RING_BD_MULTIPLE);
 	old_tx_desc_num = priv->ring[0].desc_num;
 	old_rx_desc_num = priv->ring[queue_num].desc_num;
+	old_rx_buf_len = priv->ring[queue_num].buf_size;
 	if (old_tx_desc_num == new_tx_desc_num &&
-	    old_rx_desc_num == new_rx_desc_num)
+	    old_rx_desc_num == new_rx_desc_num &&
+	    param_ext->rx_buf_len == old_rx_buf_len)
 		return 0;
 
 	tmp_rings = hns3_backup_ringparam(priv);
@@ -1115,19 +1144,22 @@ static int hns3_set_ringparam(struct net_device *ndev,
 	}
 
 	netdev_info(ndev,
-		    "Changing Tx/Rx ring depth from %u/%u to %u/%u\n",
+		    "Changing Tx/Rx ring depth from %u/%u to %u/%u, Changing rx buffer len from %d to %d\n",
 		    old_tx_desc_num, old_rx_desc_num,
-		    new_tx_desc_num, new_rx_desc_num);
+		    new_tx_desc_num, new_rx_desc_num,
+		    old_rx_buf_len, param_ext->rx_buf_len);
 
 	if (if_running)
 		ndev->netdev_ops->ndo_stop(ndev);
 
 	hns3_change_all_ring_bd_num(priv, new_tx_desc_num, new_rx_desc_num);
+	hns3_change_rx_buf_len(ndev, param_ext->rx_buf_len);
 	ret = hns3_init_all_ring(priv);
 	if (ret) {
-		netdev_err(ndev, "Change bd num fail, revert to old value(%d)\n",
+		netdev_err(ndev, "set ringparam fail, revert to old value(%d)\n",
 			   ret);
 
+		hns3_change_rx_buf_len(ndev, old_rx_buf_len);
 		hns3_change_all_ring_bd_num(priv, old_tx_desc_num,
 					    old_rx_desc_num);
 		for (i = 0; i < h->kinfo.num_tqps * 2; i++)
@@ -1809,6 +1841,8 @@ static int hns3_set_tunable(struct net_device *netdev,
 				 ETHTOOL_COALESCE_MAX_FRAMES |		\
 				 ETHTOOL_COALESCE_USE_CQE)
 
+#define HNS3_ETHTOOL_RING	ETHTOOL_RING_USE_RX_BUF_LEN
+
 static int hns3_get_ts_info(struct net_device *netdev,
 			    struct ethtool_ts_info *info)
 {
@@ -1887,6 +1921,7 @@ static int hns3_get_link_ext_state(struct net_device *netdev,
 
 static const struct ethtool_ops hns3vf_ethtool_ops = {
 	.supported_coalesce_params = HNS3_ETHTOOL_COALESCE,
+	.supported_ring_params = HNS3_ETHTOOL_RING,
 	.get_drvinfo = hns3_get_drvinfo,
 	.get_ringparam = hns3_get_ringparam,
 	.set_ringparam = hns3_set_ringparam,
@@ -1918,6 +1953,7 @@ static const struct ethtool_ops hns3vf_ethtool_ops = {
 
 static const struct ethtool_ops hns3_ethtool_ops = {
 	.supported_coalesce_params = HNS3_ETHTOOL_COALESCE,
+	.supported_ring_params = HNS3_ETHTOOL_RING,
 	.self_test = hns3_self_test,
 	.get_drvinfo = hns3_get_drvinfo,
 	.get_link = hns3_get_link,
-- 
2.33.0

[PATCH V4 net-next 6/6] net: hns3: remove the way to set tx spare buf via module parameter

[Guangbin Huang]

From: Hao Chen <chenhao288@hisilicon.com>

The way to set tx spare buf via module parameter is not such
convenient as the way to set it via ethtool.

So,remove the way to set tx spare buf via module parameter.

Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 0dd8987d1228..bcab29d7ed9f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -53,10 +53,6 @@ static int debug = -1;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, " Network interface message level setting");
 
-static unsigned int tx_spare_buf_size;
-module_param(tx_spare_buf_size, uint, 0400);
-MODULE_PARM_DESC(tx_spare_buf_size, "Size used to allocate tx spare buffer");
-
 static unsigned int tx_sgl = 1;
 module_param(tx_sgl, uint, 0600);
 MODULE_PARM_DESC(tx_sgl, "Minimum number of frags when using dma_map_sg() to optimize the IOMMU mapping");
@@ -1041,8 +1037,7 @@ static void hns3_init_tx_spare_buffer(struct hns3_enet_ring *ring)
 	dma_addr_t dma;
 	int order;
 
-	alloc_size = tx_spare_buf_size ? tx_spare_buf_size :
-		     ring->tqp->handle->kinfo.tx_spare_buf_size;
+	alloc_size = ring->tqp->handle->kinfo.tx_spare_buf_size;
 	if (!alloc_size)
 		return;
 
-- 
2.33.0

Re: [PATCH V4 net-next 0/6] ethtool: add support to set/get tx copybreak buf size and rx buf len

[Alexander Lobakin]

From: Guangbin Huang <huangguangbin2@huawei.com>
Date: Thu, 14 Oct 2021 19:39:37 +0800

Hi there,

> From: Hao Chen <chenhao288@hisilicon.com>
> 
> This series add support to set/get tx copybreak buf size and rx buf len via
> ethtool and hns3 driver implements them.
> 
> Tx copybreak buf size is used for tx copybreak feature which for small size
> packet or frag. Use ethtool --get-tunable command to get it, and ethtool
> --set-tunable command to set it, examples are as follow:
> 
> 1. set tx spare buf size to 102400:
> $ ethtool --set-tunable eth1 tx-buf-size 102400
> 
> 2. get tx spare buf size:
> $ ethtool --get-tunable eth1 tx-buf-size
> tx-buf-size: 102400

Isn't that supposed to be changed on changing Tx copybreak value
itsef?
And what if I set Tx copybreak buf size value lower than Tx
copybreak? I see no sanity checks for this.

> Rx buf len is buffer length of each rx BD. Use ethtool -g command to get
> it, and ethtool -G command to set it, examples are as follow:
> 
> 1. set rx buf len to 4096
> $ ethtool -G eth1 rx-buf-len 4096
> 
> 2. get rx buf len
> $ ethtool -g eth1
> ...
> RX Buf Len:     4096

Isn't that supposed to be changed on changing MTU?
And what if I set Rx buf len value lower than MTU? I see no checks
as well.

That means, do we _really_ need two new tunables?

> Change log:
> V3 -> V4
> 1.Fix a few allmodconfig compile warning.
> 2.Add more '=' synbol to ethtool-netlink.rst to refine format.
> 3.Move definement of struct ethtool_ringparam_ext to include/linux/ethtool.h.
> 4.Move related modify of rings_fill_reply() from patch 4/6 to patch 3/6.
> 
> V2 -> V3
> 1.Remove documentation for tx copybreak buf size, there is description for
> it in userspace ethtool.
> 2.Move extending parameters for get/set_ringparam function from patch3/6
> to patch 4/6.
> 
> V1 -> V2
> 1.Add documentation for rx buf len and tx copybreak buf size.
> 2.Extend structure ringparam_ext for extenal ring params.
> 3.Change type of ETHTOOL_A_RINGS_RX_BUF_LEN from NLA_U32 to
>   NLA_POLICY_MIN(NLA_U32, 1).
> 4.Add supported_ring_params in ethtool_ops to indicate if support external
>   params.
> 

[snip]

Thanks,
Al

Re: [PATCH V4 net-next 1/6] ethtool: add support to set/get tx copybreak buf size via ethtool

[Eric Dumazet]

On 10/14/21 4:39 AM, Guangbin Huang wrote:
> From: Hao Chen <chenhao288@hisilicon.com>
> 
> Add support for ethtool to set/get tx copybreak buf size.

What is the unit ?

Frankly, having to size the 'buffer' based on number of slots in TX ring buffer
is not good.

What happens later when/if ethtool -G tx xxxxx' is trying
to change number of slots ?

The 'tx copybreak' should instead give a number of bytes per TX ring slot.

Eg, 128 or 256 bytes.

This is very similar to what drivers using net/core/tso.c do.

They usually use TSO_HEADER_SIZE for this.

> 
> Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
> ---
>  include/uapi/linux/ethtool.h | 1 +
>  net/ethtool/common.c         | 1 +
>  net/ethtool/ioctl.c          | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index a2223b685451..7bc4b8def12c 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -231,6 +231,7 @@ enum tunable_id {
>  	ETHTOOL_RX_COPYBREAK,
>  	ETHTOOL_TX_COPYBREAK,
>  	ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
> +	ETHTOOL_TX_COPYBREAK_BUF_SIZE,
>  	/*
>  	 * Add your fresh new tunable attribute above and remember to update
>  	 * tunable_strings[] in net/ethtool/common.c
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index c63e0739dc6a..0c5210015911 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -89,6 +89,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
>  	[ETHTOOL_RX_COPYBREAK]	= "rx-copybreak",
>  	[ETHTOOL_TX_COPYBREAK]	= "tx-copybreak",
>  	[ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
> +	[ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
>  };
>  
>  const char
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index bf6e8c2f9bf7..617ebc4183d9 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2383,6 +2383,7 @@ static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
>  	switch (tuna->id) {
>  	case ETHTOOL_RX_COPYBREAK:
>  	case ETHTOOL_TX_COPYBREAK:
> +	case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
>  		if (tuna->len != sizeof(u32) ||
>  		    tuna->type_id != ETHTOOL_TUNABLE_U32)
>  			return -EINVAL;
> 

Re: [PATCH V4 net-next 0/6] ethtool: add support to set/get tx copybreak buf size and rx buf len

[Jakub Kicinski]

On Thu, 14 Oct 2021 15:14:20 +0200 Alexander Lobakin wrote:
> > Rx buf len is buffer length of each rx BD. Use ethtool -g command to get
> > it, and ethtool -G command to set it, examples are as follow:
> > 
> > 1. set rx buf len to 4096
> > $ ethtool -G eth1 rx-buf-len 4096
> > 
> > 2. get rx buf len
> > $ ethtool -g eth1
> > ...
> > RX Buf Len:     4096  
> 
> Isn't that supposed to be changed on changing MTU?
> And what if I set Rx buf len value lower than MTU? I see no checks
> as well.

Presumably the NIC does scatter.

> That means, do we _really_ need two new tunables?

nit let's not say tunable, "tunable" has an API meaning this one
is part of the ring param command.

The question of "why do we need this" seems fair tho - Guangbin,
can you share examples of workloads which benefit from 2k vs 4k?

Re: [PATCH V4 net-next 1/6] ethtool: add support to set/get tx copybreak buf size via ethtool

[huangguangbin (A)]

On 2021/10/14 21:58, Eric Dumazet wrote:
> 
> 
> On 10/14/21 4:39 AM, Guangbin Huang wrote:
>> From: Hao Chen <chenhao288@hisilicon.com>
>>
>> Add support for ethtool to set/get tx copybreak buf size.
> 
> What is the unit ?
> 
> Frankly, having to size the 'buffer' based on number of slots in TX ring buffer
> is not good.
> 
> What happens later when/if ethtool -G tx xxxxx' is trying
> to change number of slots ?
> 
> The 'tx copybreak' should instead give a number of bytes per TX ring slot.
> 
> Eg, 128 or 256 bytes.
> 
> This is very similar to what drivers using net/core/tso.c do.
> 
> They usually use TSO_HEADER_SIZE for this.
> 
Hi Eric,
The unit is bytes.

Tx copybreak buf is a queue based tx shared bounce buffer and it is allocated
based on page size, not based on slot numbers. When the len of xmitted skb is
below tx_copybreak, tx copybreak buf will share buffer of specific size(slot size)
for tx to memcpy the small packet.

So, what you mentioned seems not related to this patch.

>>
>> Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
>> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
>> ---
>>   include/uapi/linux/ethtool.h | 1 +
>>   net/ethtool/common.c         | 1 +
>>   net/ethtool/ioctl.c          | 1 +
>>   3 files changed, 3 insertions(+)
>>
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index a2223b685451..7bc4b8def12c 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -231,6 +231,7 @@ enum tunable_id {
>>   	ETHTOOL_RX_COPYBREAK,
>>   	ETHTOOL_TX_COPYBREAK,
>>   	ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
>> +	ETHTOOL_TX_COPYBREAK_BUF_SIZE,
>>   	/*
>>   	 * Add your fresh new tunable attribute above and remember to update
>>   	 * tunable_strings[] in net/ethtool/common.c
>> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
>> index c63e0739dc6a..0c5210015911 100644
>> --- a/net/ethtool/common.c
>> +++ b/net/ethtool/common.c
>> @@ -89,6 +89,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
>>   	[ETHTOOL_RX_COPYBREAK]	= "rx-copybreak",
>>   	[ETHTOOL_TX_COPYBREAK]	= "tx-copybreak",
>>   	[ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
>> +	[ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
>>   };
>>   
>>   const char
>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>> index bf6e8c2f9bf7..617ebc4183d9 100644
>> --- a/net/ethtool/ioctl.c
>> +++ b/net/ethtool/ioctl.c
>> @@ -2383,6 +2383,7 @@ static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
>>   	switch (tuna->id) {
>>   	case ETHTOOL_RX_COPYBREAK:
>>   	case ETHTOOL_TX_COPYBREAK:
>> +	case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
>>   		if (tuna->len != sizeof(u32) ||
>>   		    tuna->type_id != ETHTOOL_TUNABLE_U32)
>>   			return -EINVAL;
>>
> .
> 

Re: [PATCH V4 net-next 0/6] ethtool: add support to set/get tx copybreak buf size and rx buf len

[huangguangbin (A)]

On 2021/10/14 21:14, Alexander Lobakin wrote:
> From: Guangbin Huang <huangguangbin2@huawei.com>
> Date: Thu, 14 Oct 2021 19:39:37 +0800
> 
> Hi there,
> 
>> From: Hao Chen <chenhao288@hisilicon.com>
>>
>> This series add support to set/get tx copybreak buf size and rx buf len via
>> ethtool and hns3 driver implements them.
>>
>> Tx copybreak buf size is used for tx copybreak feature which for small size
>> packet or frag. Use ethtool --get-tunable command to get it, and ethtool
>> --set-tunable command to set it, examples are as follow:
>>
>> 1. set tx spare buf size to 102400:
>> $ ethtool --set-tunable eth1 tx-buf-size 102400
>>
>> 2. get tx spare buf size:
>> $ ethtool --get-tunable eth1 tx-buf-size
>> tx-buf-size: 102400
> 
> Isn't that supposed to be changed on changing Tx copybreak value
> itsef?
> And what if I set Tx copybreak buf size value lower than Tx
> copybreak? I see no sanity checks for this.
> 
Hi Alexander,
HNS3 driver still execute the previous packet sending process
when tx copybreak buf size is lower than tx copybreak,
So, we have no check for tx copybreak buf size when set it.

>> Rx buf len is buffer length of each rx BD. Use ethtool -g command to get
>> it, and ethtool -G command to set it, examples are as follow:
>>
>> 1. set rx buf len to 4096
>> $ ethtool -G eth1 rx-buf-len 4096
>>
>> 2. get rx buf len
>> $ ethtool -g eth1
>> ...
>> RX Buf Len:     4096
> 
> Isn't that supposed to be changed on changing MTU?
> And what if I set Rx buf len value lower than MTU? I see no checks
> as well.
> 
> That means, do we _really_ need two new tunables?
> 
When MTU remains unchanged and rx buf len changes from 2048 to 4096.
TCP packet receiving bandwidth of single thread changes from 34.1 Gbps to 48.6 Gbps(improve 42.5%).
it means that when we enable hardware GRO, it make sense to change rx buf len.

And if we set rx buf len(only support 2k, 4k) which is lower than MTU,
it will split packages into smaller chunks to rx bd, and in general situations,
rx buf len is greater than MTU.

>> Change log:
>> V3 -> V4
>> 1.Fix a few allmodconfig compile warning.
>> 2.Add more '=' synbol to ethtool-netlink.rst to refine format.
>> 3.Move definement of struct ethtool_ringparam_ext to include/linux/ethtool.h.
>> 4.Move related modify of rings_fill_reply() from patch 4/6 to patch 3/6.
>>
>> V2 -> V3
>> 1.Remove documentation for tx copybreak buf size, there is description for
>> it in userspace ethtool.
>> 2.Move extending parameters for get/set_ringparam function from patch3/6
>> to patch 4/6.
>>
>> V1 -> V2
>> 1.Add documentation for rx buf len and tx copybreak buf size.
>> 2.Extend structure ringparam_ext for extenal ring params.
>> 3.Change type of ETHTOOL_A_RINGS_RX_BUF_LEN from NLA_U32 to
>>    NLA_POLICY_MIN(NLA_U32, 1).
>> 4.Add supported_ring_params in ethtool_ops to indicate if support external
>>    params.
>>
> 
> [snip]
> 
> Thanks,
> Al
> .
> 

Re: [PATCH V4 net-next 0/6] ethtool: add support to set/get tx copybreak buf size and rx buf len

[Alexander Lobakin]

From: huangguangbin (A) <huangguangbin2@huawei.com>
Date: Tue, 19 Oct 2021 21:54:24 +0800

> On 2021/10/14 21:14, Alexander Lobakin wrote:
> > From: Guangbin Huang <huangguangbin2@huawei.com>
> > Date: Thu, 14 Oct 2021 19:39:37 +0800
> > 
> > Hi there,
> > 
> >> From: Hao Chen <chenhao288@hisilicon.com>
> >>
> >> This series add support to set/get tx copybreak buf size and rx buf len via
> >> ethtool and hns3 driver implements them.
> >>
> >> Tx copybreak buf size is used for tx copybreak feature which for small size
> >> packet or frag. Use ethtool --get-tunable command to get it, and ethtool
> >> --set-tunable command to set it, examples are as follow:
> >>
> >> 1. set tx spare buf size to 102400:
> >> $ ethtool --set-tunable eth1 tx-buf-size 102400
> >>
> >> 2. get tx spare buf size:
> >> $ ethtool --get-tunable eth1 tx-buf-size
> >> tx-buf-size: 102400
> > 
> > Isn't that supposed to be changed on changing Tx copybreak value
> > itsef?
> > And what if I set Tx copybreak buf size value lower than Tx
> > copybreak? I see no sanity checks for this.
> > 
> Hi Alexander,

Hi,

> HNS3 driver still execute the previous packet sending process
> when tx copybreak buf size is lower than tx copybreak,
> So, we have no check for tx copybreak buf size when set it.

So it doesn't make sense then to have two separate parameters
I believe? Just change Tx copybreak buffer size according to
the current Tx copybreak value. Any usecases when it's really
handy to have them differing in values?

> >> Rx buf len is buffer length of each rx BD. Use ethtool -g command to get
> >> it, and ethtool -G command to set it, examples are as follow:
> >>
> >> 1. set rx buf len to 4096
> >> $ ethtool -G eth1 rx-buf-len 4096
> >>
> >> 2. get rx buf len
> >> $ ethtool -g eth1
> >> ...
> >> RX Buf Len:     4096
> > 
> > Isn't that supposed to be changed on changing MTU?
> > And what if I set Rx buf len value lower than MTU? I see no checks
> > as well.
> > 
> > That means, do we _really_ need two new tunables?
> > 
> When MTU remains unchanged and rx buf len changes from 2048 to 4096.
> TCP packet receiving bandwidth of single thread changes from 34.1 Gbps to 48.6 Gbps(improve 42.5%).
> it means that when we enable hardware GRO, it make sense to change rx buf len.
> 
> And if we set rx buf len(only support 2k, 4k) which is lower than MTU,

You say your HW only supports 2k and 4k buffer size, would it
make sense if you just set it automatically depending on MTU
and NETIF_F_GRO_HW (or _LRO) to keep performance in its best
not depending on any side priv parameters that user is likely
not aware of?

> it will split packages into smaller chunks to rx bd, and in general situations,
> rx buf len is greater than MTU.
> 
> >> Change log:
> >> V3 -> V4
> >> 1.Fix a few allmodconfig compile warning.
> >> 2.Add more '=' synbol to ethtool-netlink.rst to refine format.
> >> 3.Move definement of struct ethtool_ringparam_ext to include/linux/ethtool.h.
> >> 4.Move related modify of rings_fill_reply() from patch 4/6 to patch 3/6.
> >>
> >> V2 -> V3
> >> 1.Remove documentation for tx copybreak buf size, there is description for
> >> it in userspace ethtool.
> >> 2.Move extending parameters for get/set_ringparam function from patch3/6
> >> to patch 4/6.
> >>
> >> V1 -> V2
> >> 1.Add documentation for rx buf len and tx copybreak buf size.
> >> 2.Extend structure ringparam_ext for extenal ring params.
> >> 3.Change type of ETHTOOL_A_RINGS_RX_BUF_LEN from NLA_U32 to
> >>    NLA_POLICY_MIN(NLA_U32, 1).
> >> 4.Add supported_ring_params in ethtool_ops to indicate if support external
> >>    params.
> >>
> > 
> > [snip]
> > 
> > Thanks,
> > Al

Thanks,
Al

Re: [PATCH V4 net-next 0/6] ethtool: add support to set/get tx copybreak buf size and rx buf len

[huangguangbin (A)]

On 2021/10/20 18:02, Alexander Lobakin wrote:
> From: huangguangbin (A) <huangguangbin2@huawei.com>
> Date: Tue, 19 Oct 2021 21:54:24 +0800
> 
>> On 2021/10/14 21:14, Alexander Lobakin wrote:
>>> From: Guangbin Huang <huangguangbin2@huawei.com>
>>> Date: Thu, 14 Oct 2021 19:39:37 +0800
>>>
>>> Hi there,
>>>
>>>> From: Hao Chen <chenhao288@hisilicon.com>
>>>>
>>>> This series add support to set/get tx copybreak buf size and rx buf len via
>>>> ethtool and hns3 driver implements them.
>>>>
>>>> Tx copybreak buf size is used for tx copybreak feature which for small size
>>>> packet or frag. Use ethtool --get-tunable command to get it, and ethtool
>>>> --set-tunable command to set it, examples are as follow:
>>>>
>>>> 1. set tx spare buf size to 102400:
>>>> $ ethtool --set-tunable eth1 tx-buf-size 102400
>>>>
>>>> 2. get tx spare buf size:
>>>> $ ethtool --get-tunable eth1 tx-buf-size
>>>> tx-buf-size: 102400
>>>
>>> Isn't that supposed to be changed on changing Tx copybreak value
>>> itsef?
>>> And what if I set Tx copybreak buf size value lower than Tx
>>> copybreak? I see no sanity checks for this.
>>>
>> Hi Alexander,
> 
> Hi,
> 
>> HNS3 driver still execute the previous packet sending process
>> when tx copybreak buf size is lower than tx copybreak,
>> So, we have no check for tx copybreak buf size when set it.
> 
> So it doesn't make sense then to have two separate parameters
> I believe? Just change Tx copybreak buffer size according to
> the current Tx copybreak value. Any usecases when it's really
> handy to have them differing in values?
> 
Hi Alexander,
Do you mean allocate tx copybreak buf based on tx copybreak such as the size
tx_copybreak * bd_num * ring_num?

In this way, if we set a large value for BD number(the max BD number of hns3
driver is 37260) of each ring(the max ring number of hns3 driver is 1280 now),
the driver will allocate a huge memory. It may cause memory waste if the driver
actually use very few tx copybreak buf in the case of sending low load of small
packets.

So we think it is better to decide the tx copybreak size by customers according
to their usecase, and all BDs of each ring share the same tx copybreak buf.



>>>> Rx buf len is buffer length of each rx BD. Use ethtool -g command to get
>>>> it, and ethtool -G command to set it, examples are as follow:
>>>>
>>>> 1. set rx buf len to 4096
>>>> $ ethtool -G eth1 rx-buf-len 4096
>>>>
>>>> 2. get rx buf len
>>>> $ ethtool -g eth1
>>>> ...
>>>> RX Buf Len:     4096
>>>
>>> Isn't that supposed to be changed on changing MTU?
>>> And what if I set Rx buf len value lower than MTU? I see no checks
>>> as well.
>>>
>>> That means, do we _really_ need two new tunables?
>>>
>> When MTU remains unchanged and rx buf len changes from 2048 to 4096.
>> TCP packet receiving bandwidth of single thread changes from 34.1 Gbps to 48.6 Gbps(improve 42.5%).
>> it means that when we enable hardware GRO, it make sense to change rx buf len.
>>
>> And if we set rx buf len(only support 2k, 4k) which is lower than MTU,
> 
> You say your HW only supports 2k and 4k buffer size, would it
> make sense if you just set it automatically depending on MTU
> and NETIF_F_GRO_HW (or _LRO) to keep performance in its best
> not depending on any side priv parameters that user is likely
> not aware of?
> 
As GRO only supports TCP package, not supports UDP or tunnel package. It
means that when MTU is 1500 and receive UDP or tunnel packets, the
performance of bandwidth still can't be improved even chang rx buf len
from 2048 to 4096, it just waste memory.

So, we don't want to set rx buf len automatically according to MTU and
NETIF_F_GRO_HW. Similarly, we think it is better to decide it by customers
according to their usecase.



>> it will split packages into smaller chunks to rx bd, and in general situations,
>> rx buf len is greater than MTU.
>>
>>>> Change log:
>>>> V3 -> V4
>>>> 1.Fix a few allmodconfig compile warning.
>>>> 2.Add more '=' synbol to ethtool-netlink.rst to refine format.
>>>> 3.Move definement of struct ethtool_ringparam_ext to include/linux/ethtool.h.
>>>> 4.Move related modify of rings_fill_reply() from patch 4/6 to patch 3/6.
>>>>
>>>> V2 -> V3
>>>> 1.Remove documentation for tx copybreak buf size, there is description for
>>>> it in userspace ethtool.
>>>> 2.Move extending parameters for get/set_ringparam function from patch3/6
>>>> to patch 4/6.
>>>>
>>>> V1 -> V2
>>>> 1.Add documentation for rx buf len and tx copybreak buf size.
>>>> 2.Extend structure ringparam_ext for extenal ring params.
>>>> 3.Change type of ETHTOOL_A_RINGS_RX_BUF_LEN from NLA_U32 to
>>>>     NLA_POLICY_MIN(NLA_U32, 1).
>>>> 4.Add supported_ring_params in ethtool_ops to indicate if support external
>>>>     params.
>>>>
>>>
>>> [snip]
>>>
>>> Thanks,
>>> Al
> 
> Thanks,
> Al
> .
> 

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[Marc Kleine-Budde]

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

On 14.10.2021 19:39:41, Guangbin Huang wrote:
> From: Hao Chen <chenhao288@hisilicon.com>
> 
> Add two new parameters ringparam_ext and extack for
> .get_ringparam and .set_ringparam to extend more ring params
> through netlink.
> 
> Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>

While discussing a different ethtool ring param extension, Andrew Lunn
suggested a different approach to extend the get/set_ringparam
callbacks. See:

https://lore.kernel.org/all/YXaimhlXkpBKRQin@lunn.ch/

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[Marc Kleine-Budde]

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

On 25.10.2021 15:11:49, Marc Kleine-Budde wrote:
> On 14.10.2021 19:39:41, Guangbin Huang wrote:
> > From: Hao Chen <chenhao288@hisilicon.com>
> > 
> > Add two new parameters ringparam_ext and extack for
> > .get_ringparam and .set_ringparam to extend more ring params
> > through netlink.
> > 
> > Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
> > Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
> 
> While discussing a different ethtool ring param extension,

Let me explain my requirements:

There is a not Ethernet based bus system, called CAN (mainly used in the
automotive and industrial world). It comes in 2 different generations or
modes (CAN-2.0 and CAN-FD) and the 3rd one CAN-XL has already been
specified.

Due to different frame sizes used in these CAN modes and HW limitations,
we need the possibility to set a RX/TX ring configuration for each of
these modes.

The approach Andrew suggested is two-fold. First introduce a "struct
ethtool_kringparam" that's only used inside the kernel, as "struct
ethtool_ringparam" is ABI. Then extend "struct ethtool_kringparam" as
needed.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[Jakub Kicinski]

On Mon, 25 Oct 2021 15:27:18 +0200 Marc Kleine-Budde wrote:
> On 25.10.2021 15:11:49, Marc Kleine-Budde wrote:
> > On 14.10.2021 19:39:41, Guangbin Huang wrote:  
> > > From: Hao Chen <chenhao288@hisilicon.com>
> > > 
> > > Add two new parameters ringparam_ext and extack for
> > > .get_ringparam and .set_ringparam to extend more ring params
> > > through netlink.
> > > 
> > > Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
> > > Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>  
> > 
> > While discussing a different ethtool ring param extension,  
> 
> Let me explain my requirements:
> 
> There is a not Ethernet based bus system, called CAN (mainly used in the
> automotive and industrial world). It comes in 2 different generations or
> modes (CAN-2.0 and CAN-FD) and the 3rd one CAN-XL has already been
> specified.
> 
> Due to different frame sizes used in these CAN modes and HW limitations,
> we need the possibility to set a RX/TX ring configuration for each of
> these modes.
> 
> The approach Andrew suggested is two-fold. First introduce a "struct
> ethtool_kringparam" that's only used inside the kernel, as "struct
> ethtool_ringparam" is ABI. Then extend "struct ethtool_kringparam" as
> needed.

Indeed, there are different ways to extend the API for drivers,
I think it comes down to personal taste. I find the "inheritance" 
models in C (kstruct usually contains the old struct as some "base")
awkward.

I don't think we have agreed-on best practice in the area.

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[Marc Kleine-Budde]

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

On 25.10.2021 10:45:05, Jakub Kicinski wrote:
> On Mon, 25 Oct 2021 15:27:18 +0200 Marc Kleine-Budde wrote:
> > On 25.10.2021 15:11:49, Marc Kleine-Budde wrote:
> > > On 14.10.2021 19:39:41, Guangbin Huang wrote:  
> > > > From: Hao Chen <chenhao288@hisilicon.com>
> > > > 
> > > > Add two new parameters ringparam_ext and extack for
> > > > .get_ringparam and .set_ringparam to extend more ring params
> > > > through netlink.
> > > > 
> > > > Signed-off-by: Hao Chen <chenhao288@hisilicon.com>
> > > > Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>  
> > > 
> > > While discussing a different ethtool ring param extension,  
> > 
> > Let me explain my requirements:
> > 
> > There is a not Ethernet based bus system, called CAN (mainly used in the
> > automotive and industrial world). It comes in 2 different generations or
> > modes (CAN-2.0 and CAN-FD) and the 3rd one CAN-XL has already been
> > specified.
> > 
> > Due to different frame sizes used in these CAN modes and HW limitations,
> > we need the possibility to set a RX/TX ring configuration for each of
> > these modes.
> > 
> > The approach Andrew suggested is two-fold. First introduce a "struct
> > ethtool_kringparam" that's only used inside the kernel, as "struct
> > ethtool_ringparam" is ABI. Then extend "struct ethtool_kringparam" as
> > needed.
> 
> Indeed, there are different ways to extend the API for drivers,
> I think it comes down to personal taste. I find the "inheritance" 
> models in C (kstruct usually contains the old struct as some "base")
> awkward.
> 
> I don't think we have agreed-on best practice in the area.

The set/get_coalesce as just extended, using a 3rd parameter for the new
values:

| 	int	(*set_coalesce)(struct net_device *,
| 				struct ethtool_coalesce *,
| 				struct kernel_ethtool_coalesce *,
| 				struct netlink_ext_ack *);

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=f3ccfda19319

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[Marc Kleine-Budde]

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

On 25.10.2021 10:45:05, Jakub Kicinski wrote:
> > The approach Andrew suggested is two-fold. First introduce a "struct
> > ethtool_kringparam" that's only used inside the kernel, as "struct
> > ethtool_ringparam" is ABI. Then extend "struct ethtool_kringparam" as
> > needed.
> 
> Indeed, there are different ways to extend the API for drivers,
> I think it comes down to personal taste. I find the "inheritance" 
> models in C (kstruct usually contains the old struct as some "base")
> awkward.
> 
> I don't think we have agreed-on best practice in the area.

From my point of view, if there already is an extension mainline:

| https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=f3ccfda19319

I'm more in the flavor for modeling other extensions the same way. Would
be more consistent to name the new struct "kernel_"ethtool_ringparam,
following the coalescing example:

| struct kernel_ethtool_ringparam {
|        __u32   rx_buf_len;
| };

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[huangguangbin (A)]

On 2021/10/26 3:01, Marc Kleine-Budde wrote:
> On 25.10.2021 10:45:05, Jakub Kicinski wrote:
>>> The approach Andrew suggested is two-fold. First introduce a "struct
>>> ethtool_kringparam" that's only used inside the kernel, as "struct
>>> ethtool_ringparam" is ABI. Then extend "struct ethtool_kringparam" as
>>> needed.
>>
>> Indeed, there are different ways to extend the API for drivers,
>> I think it comes down to personal taste. I find the "inheritance"
>> models in C (kstruct usually contains the old struct as some "base")
>> awkward.
>>
>> I don't think we have agreed-on best practice in the area.
> 
>  From my point of view, if there already is an extension mainline:
> 
> | https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=f3ccfda19319
> 
> I'm more in the flavor for modeling other extensions the same way. Would
> be more consistent to name the new struct "kernel_"ethtool_ringparam,
> following the coalescing example:
> 
> | struct kernel_ethtool_ringparam {
> |        __u32   rx_buf_len;
> | };
> 
> regards,
> Marc
> 
We think ethtool_ringparam_ext is more easy to understand it is extension of
struct ethtool_ringparam. However, we don't mind to keep the same way and modify
to the name kernel_ethtool_ringparam if everyone agrees.

Does anyone have other opinions?

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[Jakub Kicinski]

On Tue, 26 Oct 2021 22:41:19 +0800 huangguangbin (A) wrote:
> On 2021/10/26 3:01, Marc Kleine-Budde wrote:
> > On 25.10.2021 10:45:05, Jakub Kicinski wrote:  
> >> Indeed, there are different ways to extend the API for drivers,
> >> I think it comes down to personal taste. I find the "inheritance"
> >> models in C (kstruct usually contains the old struct as some "base")
> >> awkward.
> >>
> >> I don't think we have agreed-on best practice in the area.  
> > 
> >  From my point of view, if there already is an extension mainline:
> > 
> > | https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=f3ccfda19319
> > 
> > I'm more in the flavor for modeling other extensions the same way. Would
> > be more consistent to name the new struct "kernel_"ethtool_ringparam,
> > following the coalescing example:
> > 
> > | struct kernel_ethtool_ringparam {
> > |        __u32   rx_buf_len;

nit: no __, just u32. It's not uAPI.

> > | };
> > 
> > regards,
> > Marc
> >   
> We think ethtool_ringparam_ext is more easy to understand it is extension of
> struct ethtool_ringparam. However, we don't mind to keep the same way and modify
> to the name kernel_ethtool_ringparam if everyone agrees.
> 
> Does anyone have other opinions?

Either way is fine by me. Andrew's way is fine too, as long as we don't
embed the old structure into the new one but translate field-by-field.

Re: [PATCH V4 net-next 4/6] ethtool: extend ringparam setting uAPI with rx_buf_len

[Andrew Lunn]

> We think ethtool_ringparam_ext is more easy to understand it is extension of
> struct ethtool_ringparam. However, we don't mind to keep the same way and modify
> to the name kernel_ethtool_ringparam if everyone agrees.
> 
> Does anyone have other opinions?

What has been done most in the past? We should one way to do this, and
consistently use it everywhere in the network stack.

     Andrew