On 16 Nov 02:17, Taehee Yoo wrote:
On Fri, Nov 15, 2024 at 1:22 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
On Wed, 13 Nov 2024 17:32:16 +0000 Taehee Yoo wrote:
> When tcp-data-split is UNKNOWN mode, drivers arbitrarily handle it.
> For example, bnxt_en driver automatically enables if at least one of
> LRO/GRO/JUMBO is enabled.
> If tcp-data-split is UNKNOWN and LRO is enabled, a driver returns
> ENABLES of tcp-data-split, not UNKNOWN.
> So, `ethtool -g eth0` shows tcp-data-split is enabled.
>
> The problem is in the setting situation.
> In the ethnl_set_rings(), it first calls get_ringparam() to get the
> current driver's config.
> At that moment, if driver's tcp-data-split config is UNKNOWN, it returns
> ENABLE if LRO/GRO/JUMBO is enabled.
> Then, it sets values from the user and driver's current config to
> kernel_ethtool_ringparam.
> Last it calls .set_ringparam().
> The driver, especially bnxt_en driver receives
> ETHTOOL_TCP_DATA_SPLIT_ENABLED.
> But it can't distinguish whether it is set by the user or just the
> current config.
>
> The new tcp_data_split_mod member indicates the tcp-data-split value is
> explicitly set by the user.
> So the driver can handle ETHTOOL_TCP_DATA_SPLIT_ENABLED properly.
I think this can work, but it isn't exactly what I had in mind.
I was thinking we'd simply add u8 hds_config to
struct ethtool_netdev_state (which is stored inside netdev).
And update it there if user request via ethnl_set_rings() succeeds.
That gives the driver and the core quick and easy access to checking if
the user forced the setting to ENABLED or DISABLED, or didn't (UNKNOWN).
As far as the parameter passed to ->set_ringparam() goes we could do
(assuming the new fields in ethtool_netdev state is called hds):
kernel_ringparam.tcp_data_split =
nla_get_u32_default(tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT],
dev->ethtool->hds);
If the driver see UNKNOWN it means user doesn't care.
If the driver sees ENABLED/DISABLE it must comply, doesn't matter if
the user requested it in current netlink call, or previous and hasn't
reset it, yet.
This complicates things, drivers shouldn't store previous uncommitted "wanted" values.
We have wanted_features for that, and I don't think it's smart to have yet another
wanted_features mechanism, let's keep it simple, any explicit config by
ethtool should either be immediately committed or returned as error to
user and driver will only reflect the old/current value in future get requests.
HDS can conflict with many other features e.g XDP/LRO/rx_copy_break/MTU
limitations etc ...
Hope this makes sense...
Thank you so much for the details!
I will try to use ethtool_netdev_state instead of this approach.
Thanks a lot!
Taehee Yoo
