| Message ID | 20241113173222.372128-3-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: implement tcp-data-split and thresh option | expand |
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...
On Fri, Nov 15, 2024 at 1:22 PM Jakub Kicinski <kuba@kernel.org> 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
On 16 Nov 02:17, Taehee Yoo wrote: >On Fri, Nov 15, 2024 at 1:22 PM Jakub Kicinski <kuba@kernel.org> 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 >
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 1199e308c8dd..ecd52b99a63a 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -73,6 +73,7 @@ enum { * struct kernel_ethtool_ringparam - RX/TX ring configuration * @rx_buf_len: Current length of buffers on the rx ring. * @tcp_data_split: Scatter packet headers and data to separate buffers + * @tcp_data_split_mod: Updated tcp-data-split from user * @tx_push: The flag of tx push mode * @rx_push: The flag of rx push mode * @cqe_size: Size of TX/RX completion queue event @@ -82,6 +83,7 @@ enum { struct kernel_ethtool_ringparam { u32 rx_buf_len; u8 tcp_data_split; + bool tcp_data_split_mod; u8 tx_push; u8 rx_push; u32 cqe_size; diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c index b7865a14fdf8..c12ebb61394d 100644 --- a/net/ethtool/rings.c +++ b/net/ethtool/rings.c @@ -250,6 +250,9 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info) return -EINVAL; } + if (tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT]) + kernel_ringparam.tcp_data_split_mod = true; + ret = dev->ethtool_ops->set_ringparam(dev, &ringparam, &kernel_ringparam, info->extack); return ret < 0 ? ret : 1;
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. Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v5: - Patch added. include/linux/ethtool.h | 2 ++ net/ethtool/rings.c | 3 +++ 2 files changed, 5 insertions(+)