[PATCH net-next 0/3] net: ethool: add support to get/set tx push by ethtool -G/g

[PATCH net-next 0/3] net: ethool: add support to get/set tx push by ethtool -G/g

[Guangbin Huang]

From: Jie Wang <wangjie125@huawei.com>

These three patches add tx push in ring params and adapt the set and get APIs
of ring params.

ChangeLog:

RFC V4->V1
1.Add detailed description about the tx push mode
2.Modify the patch subject title
link: https://lore.kernel.org/netdev/20220331084342.27043-1-wangjie125@huawei.com/

RFC V3->RFC V4
1.Put three request checks before rtnl_lock() in ethnl_set_rings
2.Add tx push feature description in Documentation/networking/ethtool-netlink.rst
3.Use netdev_dbg to track changes in hns3_set_tx_push
link: https://lore.kernel.org/netdev/20220329091913.17869-1-wangjie125@huawei.com/

RFC V2->RFC V3
1.Add tx push documentation in Documentation/networking/ethtool-netlink.rst
2.Use u8 to store tx push in struct kernel_ethtool_ringparam
3.Add ETHTOOL_RING_USE_TX_PUSH to reject setting for unsupported driver
4.Use NLA_POLICY_MAX(NLA_U8, 1) to limit the tx push value
link: https://lore.kernel.org/netdev/20220326085102.14111-1-wangjie125@huawei.com/

RFC V1->RFC V2
1.Extend tx push param in ringparam, suggested by Jakub Kicinski.
link: https://lore.kernel.org/netdev/20220315032108.57228-1-wangjie125@huawei.com/

Jie Wang (3):
  net: ethtool: extend ringparam set/get APIs for tx_push
  net: ethtool: move checks before rtnl_lock() in ethnl_set_rings
  net: hns3: add tx push support in hns3 ring param process

 Documentation/networking/ethtool-netlink.rst  |  8 +++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    | 33 ++++++++++-
 include/linux/ethtool.h                       |  3 +
 include/uapi/linux/ethtool_netlink.h          |  1 +
 net/ethtool/netlink.h                         |  2 +-
 net/ethtool/rings.c                           | 56 ++++++++++++-------
 6 files changed, 81 insertions(+), 22 deletions(-)

-- 
2.33.0

[PATCH net-next 1/3] net: ethtool: extend ringparam set/get APIs for tx_push

[Guangbin Huang]

From: Jie Wang <wangjie125@huawei.com>

Currently tx push is a standard driver feature which controls use of a fast
path descriptor push. So this patch extends the ringparam APIs and data
structures to support set/get tx push by ethtool -G/g.

Signed-off-by: Jie Wang <wangjie125@huawei.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 Documentation/networking/ethtool-netlink.rst |  8 ++++++++
 include/linux/ethtool.h                      |  3 +++
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.h                        |  2 +-
 net/ethtool/rings.c                          | 18 ++++++++++++++++--
 5 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 24d9be69065d..fcd4fdf96c8e 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -862,6 +862,7 @@ Kernel response contents:
   ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
   ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT``    u8      TCP header / data split
   ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
+  ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
   ====================================  ======  ===========================
 
 ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable with
@@ -871,6 +872,12 @@ separate buffers. The device configuration must make it possible to receive
 full memory pages of data, for example because MTU is high enough or through
 HW-GRO.
 
+``ETHTOOL_A_RINGS_TX_PUSH`` flag is used to choose the ordinary path or the fast
+path to send packets. In ordinary path, driver fills BDs to DDR memory and
+notifies NIC hardware. In fast path, driver pushes BDs to the device memory
+directly and thus reducing the sending latencies. Setting tx push attribute "on"
+will enable tx push mode and send packets in fast path if packet size matches.
+For those not supported hardwares, this attributes is "off" by default settings.
 
 RINGS_SET
 =========
@@ -887,6 +894,7 @@ Request contents:
   ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
   ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
   ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
+  ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
   ====================================  ======  ===========================
 
 Kernel checks that requested ring sizes do not exceed limits reported by
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 4af58459a1e7..ede4f9154cd2 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -71,11 +71,13 @@ enum {
  * struct kernel_ethtool_ringparam - RX/TX ring configuration
  * @rx_buf_len: Current length of buffers on the rx ring.
  * @tcp_data_split: Scatter packet headers and data to separate buffers
+ * @tx_push: The flag of tx push mode
  * @cqe_size: Size of TX/RX completion queue event
  */
 struct kernel_ethtool_ringparam {
 	u32	rx_buf_len;
 	u8	tcp_data_split;
+	u8	tx_push;
 	u32	cqe_size;
 };
 
@@ -87,6 +89,7 @@ struct kernel_ethtool_ringparam {
 enum ethtool_supported_ring_param {
 	ETHTOOL_RING_USE_RX_BUF_LEN = BIT(0),
 	ETHTOOL_RING_USE_CQE_SIZE   = BIT(1),
+	ETHTOOL_RING_USE_TX_PUSH    = BIT(2),
 };
 
 #define __ETH_RSS_HASH_BIT(bit)	((u32)1 << (bit))
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 979850221b8d..d2fb4f7be61b 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -338,6 +338,7 @@ enum {
 	ETHTOOL_A_RINGS_RX_BUF_LEN,                     /* u32 */
 	ETHTOOL_A_RINGS_TCP_DATA_SPLIT,			/* u8 */
 	ETHTOOL_A_RINGS_CQE_SIZE,			/* u32 */
+	ETHTOOL_A_RINGS_TX_PUSH,			/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_RINGS_CNT,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 29d01662a48b..7919ddb2371c 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -363,7 +363,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_CQE_SIZE + 1];
+extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_TX_PUSH + 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 9f33c9689b56..9ed60c507d97 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -55,7 +55,8 @@ static int rings_reply_size(const struct ethnl_req_info *req_base,
 	       nla_total_size(sizeof(u32)) +	/* _RINGS_TX */
 	       nla_total_size(sizeof(u32)) +	/* _RINGS_RX_BUF_LEN */
 	       nla_total_size(sizeof(u8))  +	/* _RINGS_TCP_DATA_SPLIT */
-	       nla_total_size(sizeof(u32));	/* _RINGS_CQE_SIZE */
+	       nla_total_size(sizeof(u32)  +	/* _RINGS_CQE_SIZE */
+	       nla_total_size(sizeof(u8)));	/* _RINGS_TX_PUSH */
 }
 
 static int rings_fill_reply(struct sk_buff *skb,
@@ -94,7 +95,8 @@ static int rings_fill_reply(struct sk_buff *skb,
 	     (nla_put_u8(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT,
 			 kr->tcp_data_split))) ||
 	    (kr->cqe_size &&
-	     (nla_put_u32(skb, ETHTOOL_A_RINGS_CQE_SIZE, kr->cqe_size))))
+	     (nla_put_u32(skb, ETHTOOL_A_RINGS_CQE_SIZE, kr->cqe_size))) ||
+	    nla_put_u8(skb, ETHTOOL_A_RINGS_TX_PUSH, !!kr->tx_push))
 		return -EMSGSIZE;
 
 	return 0;
@@ -123,6 +125,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
 	[ETHTOOL_A_RINGS_TX]			= { .type = NLA_U32 },
 	[ETHTOOL_A_RINGS_RX_BUF_LEN]            = NLA_POLICY_MIN(NLA_U32, 1),
 	[ETHTOOL_A_RINGS_CQE_SIZE]		= NLA_POLICY_MIN(NLA_U32, 1),
+	[ETHTOOL_A_RINGS_TX_PUSH]		= NLA_POLICY_MAX(NLA_U8, 1),
 };
 
 int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
@@ -149,6 +152,15 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
 	if (!ops->get_ringparam || !ops->set_ringparam)
 		goto out_dev;
 
+	if (tb[ETHTOOL_A_RINGS_TX_PUSH] &&
+	    !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) {
+		ret = -EOPNOTSUPP;
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_RINGS_TX_PUSH],
+				    "setting tx push not supported");
+		goto out_dev;
+	}
+
 	rtnl_lock();
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
@@ -165,6 +177,8 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
 			 tb[ETHTOOL_A_RINGS_RX_BUF_LEN], &mod);
 	ethnl_update_u32(&kernel_ringparam.cqe_size,
 			 tb[ETHTOOL_A_RINGS_CQE_SIZE], &mod);
+	ethnl_update_u8(&kernel_ringparam.tx_push,
+			tb[ETHTOOL_A_RINGS_TX_PUSH], &mod);
 	ret = 0;
 	if (!mod)
 		goto out_ops;
-- 
2.33.0

[PATCH net-next 2/3] net: ethtool: move checks before rtnl_lock() in ethnl_set_rings

[Guangbin Huang]

From: Jie Wang <wangjie125@huawei.com>

Currently these two checks in ethnl_set_rings are added after rtnl_lock()
which will do useless works if the request is invalid.

So this patch moves these checks before the rtnl_lock() to avoid these
costs.

Signed-off-by: Jie Wang <wangjie125@huawei.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 net/ethtool/rings.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 9ed60c507d97..46415d8fc256 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -152,6 +152,26 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
 	if (!ops->get_ringparam || !ops->set_ringparam)
 		goto out_dev;
 
+	if (tb[ETHTOOL_A_RINGS_RX_BUF_LEN] &&
+	    nla_get_u32(tb[ETHTOOL_A_RINGS_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_dev;
+	}
+
+	if (tb[ETHTOOL_A_RINGS_CQE_SIZE] &&
+	    nla_get_u32(tb[ETHTOOL_A_RINGS_CQE_SIZE]) &&
+	    !(ops->supported_ring_params & ETHTOOL_RING_USE_CQE_SIZE)) {
+		ret = -EOPNOTSUPP;
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_RINGS_CQE_SIZE],
+				    "setting cqe size not supported");
+		goto out_dev;
+	}
+
 	if (tb[ETHTOOL_A_RINGS_TX_PUSH] &&
 	    !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) {
 		ret = -EOPNOTSUPP;
@@ -201,24 +221,6 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
 		goto out_ops;
 	}
 
-	if (kernel_ringparam.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;
-	}
-
-	if (kernel_ringparam.cqe_size &&
-	    !(ops->supported_ring_params & ETHTOOL_RING_USE_CQE_SIZE)) {
-		ret = -EOPNOTSUPP;
-		NL_SET_ERR_MSG_ATTR(info->extack,
-				    tb[ETHTOOL_A_RINGS_CQE_SIZE],
-				    "setting cqe size not supported");
-		goto out_ops;
-	}
-
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, info->extack);
 	if (ret < 0)
-- 
2.33.0

[PATCH net-next 3/3] net: hns3: add tx push support in hns3 ring param process

[Guangbin Huang]

From: Jie Wang <wangjie125@huawei.com>

This patch adds tx push param to hns3 ring param and adapts the set and get
API of ring params. So users can set it by cmd ethtool -G and get it by cmd
ethtool -g.

Signed-off-by: Jie Wang <wangjie125@huawei.com>
Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index f4da77452126..9f4111fd2986 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -664,6 +664,8 @@ static void hns3_get_ringparam(struct net_device *netdev,
 	param->tx_pending = priv->ring[0].desc_num;
 	param->rx_pending = priv->ring[rx_queue_index].desc_num;
 	kernel_param->rx_buf_len = priv->ring[rx_queue_index].buf_size;
+	kernel_param->tx_push = test_bit(HNS3_NIC_STATE_TX_PUSH_ENABLE,
+					 &priv->state);
 }
 
 static void hns3_get_pauseparam(struct net_device *netdev,
@@ -1120,6 +1122,30 @@ static int hns3_change_rx_buf_len(struct net_device *ndev, u32 rx_buf_len)
 	return 0;
 }
 
+static int hns3_set_tx_push(struct net_device *netdev, u32 tx_push)
+{
+	struct hns3_nic_priv *priv = netdev_priv(netdev);
+	struct hnae3_handle *h = hns3_get_handle(netdev);
+	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(h->pdev);
+	u32 old_state = test_bit(HNS3_NIC_STATE_TX_PUSH_ENABLE, &priv->state);
+
+	if (!test_bit(HNAE3_DEV_SUPPORT_TX_PUSH_B, ae_dev->caps) && tx_push)
+		return -EOPNOTSUPP;
+
+	if (tx_push == old_state)
+		return 0;
+
+	netdev_dbg(netdev, "Changing tx push from %s to %s\n",
+		   old_state ? "on" : "off", tx_push ? "on" : "off");
+
+	if (tx_push)
+		set_bit(HNS3_NIC_STATE_TX_PUSH_ENABLE, &priv->state);
+	else
+		clear_bit(HNS3_NIC_STATE_TX_PUSH_ENABLE, &priv->state);
+
+	return 0;
+}
+
 static int hns3_set_ringparam(struct net_device *ndev,
 			      struct ethtool_ringparam *param,
 			      struct kernel_ethtool_ringparam *kernel_param,
@@ -1139,6 +1165,10 @@ static int hns3_set_ringparam(struct net_device *ndev,
 	if (ret)
 		return ret;
 
+	ret = hns3_set_tx_push(ndev, kernel_param->tx_push);
+	if (ret)
+		return ret;
+
 	/* Hardware requires that its descriptors must be multiple of eight */
 	new_tx_desc_num = ALIGN(param->tx_pending, HNS3_RING_BD_MULTIPLE);
 	new_rx_desc_num = ALIGN(param->rx_pending, HNS3_RING_BD_MULTIPLE);
@@ -1858,7 +1888,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
+#define HNS3_ETHTOOL_RING	(ETHTOOL_RING_USE_RX_BUF_LEN |		\
+				 ETHTOOL_RING_USE_TX_PUSH)
 
 static int hns3_get_ts_info(struct net_device *netdev,
 			    struct ethtool_ts_info *info)
-- 
2.33.0

Re: [PATCH net-next 1/3] net: ethtool: extend ringparam set/get APIs for tx_push

[Jakub Kicinski]

On Fri, 8 Apr 2022 15:12:43 +0800 Guangbin Huang wrote:
> From: Jie Wang <wangjie125@huawei.com>
> 
> Currently tx push is a standard driver feature which controls use of a fast
> path descriptor push. So this patch extends the ringparam APIs and data
> structures to support set/get tx push by ethtool -G/g.
> 
> Signed-off-by: Jie Wang <wangjie125@huawei.com>
> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>

> +``ETHTOOL_A_RINGS_TX_PUSH`` flag is used to choose the ordinary path or the fast
> +path to send packets. In ordinary path, driver fills BDs to DDR memory and
> +notifies NIC hardware. In fast path, driver pushes BDs to the device memory
> +directly and thus reducing the sending latencies. Setting tx push attribute "on"
> +will enable tx push mode and send packets in fast path if packet size matches.
> +For those not supported hardwares, this attributes is "off" by default settings.

Since you need to respin to fix the kdoc warning - could you also add 
a mention that enabling this feature may increase CPU cost? Unless it's
not the case for your implementation, I thought it usually is..

>  RINGS_SET
>  =========
> @@ -887,6 +894,7 @@ Request contents:
>    ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
>    ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
>    ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
> +  ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
>    ====================================  ======  ===========================
>  
>  Kernel checks that requested ring sizes do not exceed limits reported by
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 4af58459a1e7..ede4f9154cd2 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -71,11 +71,13 @@ enum {
>   * struct kernel_ethtool_ringparam - RX/TX ring configuration
>   * @rx_buf_len: Current length of buffers on the rx ring.
>   * @tcp_data_split: Scatter packet headers and data to separate buffers
> + * @tx_push: The flag of tx push mode
>   * @cqe_size: Size of TX/RX completion queue event
>   */
>  struct kernel_ethtool_ringparam {
>  	u32	rx_buf_len;
>  	u8	tcp_data_split;
> +	u8	tx_push;
>  	u32	cqe_size;
>  };
>  
> @@ -87,6 +89,7 @@ struct kernel_ethtool_ringparam {
>  enum ethtool_supported_ring_param {
>  	ETHTOOL_RING_USE_RX_BUF_LEN = BIT(0),
>  	ETHTOOL_RING_USE_CQE_SIZE   = BIT(1),
> +	ETHTOOL_RING_USE_TX_PUSH    = BIT(2),

include/linux/ethtool.h:94: warning: Enum value 'ETHTOOL_RING_USE_TX_PUSH' not described in enum 'ethtool_supported_ring_param'

Re: [PATCH net-next 2/3] net: ethtool: move checks before rtnl_lock() in ethnl_set_rings

[Jakub Kicinski]

On Fri, 8 Apr 2022 15:12:44 +0800 Guangbin Huang wrote:
> +	if (tb[ETHTOOL_A_RINGS_RX_BUF_LEN] &&
> +	    nla_get_u32(tb[ETHTOOL_A_RINGS_RX_BUF_LEN]) != 0 &&

I think we can drop the value checking. If attribute is present and
drivers doesn't support - reject. I don't think that would require 
any changes to existing user space but please double check.

> +	    !(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_dev;
> +	}
> +
> +	if (tb[ETHTOOL_A_RINGS_CQE_SIZE] &&
> +	    nla_get_u32(tb[ETHTOOL_A_RINGS_CQE_SIZE]) &&
> +	    !(ops->supported_ring_params & ETHTOOL_RING_USE_CQE_SIZE)) {
> +		ret = -EOPNOTSUPP;
> +		NL_SET_ERR_MSG_ATTR(info->extack,
> +				    tb[ETHTOOL_A_RINGS_CQE_SIZE],
> +				    "setting cqe size not supported");
> +		goto out_dev;
> +	}

Re: [PATCH net-next 1/3] net: ethtool: extend ringparam set/get APIs for tx_push

[wangjie (L)]

On 2022/4/9 5:55, Jakub Kicinski wrote:
> On Fri, 8 Apr 2022 15:12:43 +0800 Guangbin Huang wrote:
>> From: Jie Wang <wangjie125@huawei.com>
>>
>> Currently tx push is a standard driver feature which controls use of a fast
>> path descriptor push. So this patch extends the ringparam APIs and data
>> structures to support set/get tx push by ethtool -G/g.
>>
>> Signed-off-by: Jie Wang <wangjie125@huawei.com>
>> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>
>
>> +``ETHTOOL_A_RINGS_TX_PUSH`` flag is used to choose the ordinary path or the fast
>> +path to send packets. In ordinary path, driver fills BDs to DDR memory and
>> +notifies NIC hardware. In fast path, driver pushes BDs to the device memory
>> +directly and thus reducing the sending latencies. Setting tx push attribute "on"
>> +will enable tx push mode and send packets in fast path if packet size matches.
>> +For those not supported hardwares, this attributes is "off" by default settings.
>
> Since you need to respin to fix the kdoc warning - could you also add
> a mention that enabling this feature may increase CPU cost? Unless it's
> not the case for your implementation, I thought it usually is..
>
ok, i will add it in v2
>>  RINGS_SET
>>  =========
>> @@ -887,6 +894,7 @@ Request contents:
>>    ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
>>    ``ETHTOOL_A_RINGS_RX_BUF_LEN``        u32     size of buffers on the ring
>>    ``ETHTOOL_A_RINGS_CQE_SIZE``          u32     Size of TX/RX CQE
>> +  ``ETHTOOL_A_RINGS_TX_PUSH``           u8      flag of TX Push mode
>>    ====================================  ======  ===========================
>>
>>  Kernel checks that requested ring sizes do not exceed limits reported by
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index 4af58459a1e7..ede4f9154cd2 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -71,11 +71,13 @@ enum {
>>   * struct kernel_ethtool_ringparam - RX/TX ring configuration
>>   * @rx_buf_len: Current length of buffers on the rx ring.
>>   * @tcp_data_split: Scatter packet headers and data to separate buffers
>> + * @tx_push: The flag of tx push mode
>>   * @cqe_size: Size of TX/RX completion queue event
>>   */
>>  struct kernel_ethtool_ringparam {
>>  	u32	rx_buf_len;
>>  	u8	tcp_data_split;
>> +	u8	tx_push;
>>  	u32	cqe_size;
>>  };
>>
>> @@ -87,6 +89,7 @@ struct kernel_ethtool_ringparam {
>>  enum ethtool_supported_ring_param {
>>  	ETHTOOL_RING_USE_RX_BUF_LEN = BIT(0),
>>  	ETHTOOL_RING_USE_CQE_SIZE   = BIT(1),
>> +	ETHTOOL_RING_USE_TX_PUSH    = BIT(2),
>
> include/linux/ethtool.h:94: warning: Enum value 'ETHTOOL_RING_USE_TX_PUSH' not described in enum 'ethtool_supported_ring_param'
thx, I will fix it in v2
>
>
> .
>

Re: [PATCH net-next 2/3] net: ethtool: move checks before rtnl_lock() in ethnl_set_rings

[wangjie (L)]

On 2022/4/9 5:58, Jakub Kicinski wrote:
> On Fri, 8 Apr 2022 15:12:44 +0800 Guangbin Huang wrote:
>> +	if (tb[ETHTOOL_A_RINGS_RX_BUF_LEN] &&
>> +	    nla_get_u32(tb[ETHTOOL_A_RINGS_RX_BUF_LEN]) != 0 &&
>
> I think we can drop the value checking. If attribute is present and
> drivers doesn't support - reject. I don't think that would require
> any changes to existing user space but please double check.
>
I have checked user space code and tested the delete version on my 
server, these value checking will be dropped in v2.
>> +	    !(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_dev;
>> +	}
>> +
>> +	if (tb[ETHTOOL_A_RINGS_CQE_SIZE] &&
>> +	    nla_get_u32(tb[ETHTOOL_A_RINGS_CQE_SIZE]) &&
>> +	    !(ops->supported_ring_params & ETHTOOL_RING_USE_CQE_SIZE)) {
>> +		ret = -EOPNOTSUPP;
>> +		NL_SET_ERR_MSG_ATTR(info->extack,
>> +				    tb[ETHTOOL_A_RINGS_CQE_SIZE],
>> +				    "setting cqe size not supported");
>> +		goto out_dev;
>> +	}
>
> .
>