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: 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