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