Re: [PATCH net-next 3/5] ethtool: add support to set/get rx buf len

On 2021/8/25 23:09, Jakub Kicinski wrote:

On Wed, 25 Aug 2021 14:40:53 +0800 Guangbin Huang wrote:

quoted

From: Hao Chen <redacted>

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 <redacted>
Signed-off-by: Guangbin Huang <redacted>

quoted

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 266e95e4fb33..6e26586274b3 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h

@@ -516,6 +516,7 @@ struct ethtool_coalesce {
   *	jumbo ring
   * @tx_pending: Current maximum supported number of pending entries
   *	per TX ring
+ * @rx_buf_len: Current supported size of rx ring BD buffer.

How about "Current length of buffers on the rx ring"?

Yes, thanks.

quoted

   * If the interface does not have separate RX mini and/or jumbo rings,
   * @rx_mini_max_pending and/or @rx_jumbo_max_pending will be 0.

@@ -533,6 +534,7 @@ struct ethtool_ringparam {
  	__u32	rx_mini_pending;
  	__u32	rx_jumbo_pending;
  	__u32	tx_pending;
+	__u32	rx_buf_len;
  };

You can't extend this structure, because it's used by the IOCTL
interface directly. You need to pass the new value to the drivers
in a different way. You can look at what Yufeng Mo did recently
for an example approach.

Ok, thanks your advice, we will modify this patch.

quoted

@@ -105,6 +109,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
  	[ETHTOOL_A_RINGS_RX_MINI]		= { .type = NLA_U32 },
  	[ETHTOOL_A_RINGS_RX_JUMBO]		= { .type = NLA_U32 },
  	[ETHTOOL_A_RINGS_TX]			= { .type = NLA_U32 },
+	[ETHTOOL_A_RINGS_RX_BUF_LEN]            = { .type = NLA_U32 },

We should prevent user space for passing 0 as input, so we can use it
as a special value in the kernel. NLA_POLICY_MIN()

Ok.

quoted

  };
  
  int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)

@@ -142,6 +147,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.rx_buf_len,
+			 tb[ETHTOOL_A_RINGS_RX_BUF_LEN], &mod);
  	ret = 0;
  	if (!mod)
  		goto out_ops;

We need a way of preventing drivers which don't support the new option
from just silently ignoring it. Please add a capability like
cap_link_lanes_supported and reject non-0 ETHTOOL_A_RINGS_RX_BUF_LEN
if it's not set.
.

Ok.