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

[Alexander Lobakin <alexandr.lobakin@intel.com>]

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