From 00d0f31a1ec8eb3e4d692bdf2455daf0fb865bb3 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Thu, 20 Apr 2023 16:33:02 -0700 Subject: [PATCH] net: ethtool: coalesce: try to make user settings stick twice SET_COALESCE may change operation mode and parameters in one call. Changing operation mode may cause the driver to reset the parameter values to what is a reasonable default for new operation mode. Since driver does not know which parameters come from user and which are echoed back from ->get, driver may ignore the parameters when switching operation modes. This used to be inevitable for ioctl() but in netlink we know which parameters are actually specified by the user. We could inform which parameters were set by the user but this would lead to a lot of code duplication in the drivers. Instead try to call the drivers twice if both mode and params are changed. The set method already checks if any params need updating so in case the driver did the right thing the first time around - there will be no second call to it's ->set method (only an extra call to ->get()). For mlx5 for example before this patch we'd see: # ethtool -C eth0 adaptive-rx on adaptive-tx on # ethtool -C eth0 adaptive-rx off adaptive-tx off \ tx-usecs 123 rx-usecs 123 Adaptive RX: off TX: off rx-usecs: 3 rx-frames: 32 tx-usecs: 16 tx-frames: 32 [...] After the change: # ethtool -C eth0 adaptive-rx on adaptive-tx on # ethtool -C eth0 adaptive-rx off adaptive-tx off \ tx-usecs 123 rx-usecs 123 Adaptive RX: off TX: off rx-usecs: 123 rx-frames: 32 tx-usecs: 123 tx-frames: 32 [...] This only works for netlink, so it's a small discrepancy between netlink and ioctl(). Since we anticipate most users to move to netlink I believe it's worth making their lives easier. Link: https://lore.kernel.org/r/20230420233302.944382-1-kuba@kernel.org Signed-off-by: Jakub Kicinski --- Documentation/networking/ethtool-netlink.rst | 4 +++ net/ethtool/coalesce.c | 54 ++++++++++++++++++++++------ 2 files changed, 47 insertions(+), 11 deletions(-) diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index cd0973d..2540c70 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -1099,6 +1099,10 @@ such that the corresponding bit in ``ethtool_ops::supported_coalesce_params`` is not set), regardless of their values. Driver may impose additional constraints on coalescing parameters and their values. +Compared to requests issued via the ``ioctl()`` netlink version of this request +will try harder to make sure that values specified by the user have been applied +and may call the driver twice. + PAUSE_GET ========= diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c index 443e7e6..01a59ce 100644 --- a/net/ethtool/coalesce.c +++ b/net/ethtool/coalesce.c @@ -254,13 +254,14 @@ ethnl_set_coalesce_validate(struct ethnl_req_info *req_info, } static int -ethnl_set_coalesce(struct ethnl_req_info *req_info, struct genl_info *info) +__ethnl_set_coalesce(struct ethnl_req_info *req_info, struct genl_info *info, + bool *dual_change) { struct kernel_ethtool_coalesce kernel_coalesce = {}; struct net_device *dev = req_info->dev; struct ethtool_coalesce coalesce = {}; + bool mod_mode = false, mod = false; struct nlattr **tb = info->attrs; - bool mod = false; int ret; ret = dev->ethtool_ops->get_coalesce(dev, &coalesce, &kernel_coalesce, @@ -268,6 +269,7 @@ ethnl_set_coalesce(struct ethnl_req_info *req_info, struct genl_info *info) if (ret < 0) return ret; + /* Update values */ ethnl_update_u32(&coalesce.rx_coalesce_usecs, tb[ETHTOOL_A_COALESCE_RX_USECS], &mod); ethnl_update_u32(&coalesce.rx_max_coalesced_frames, @@ -286,10 +288,6 @@ ethnl_set_coalesce(struct ethnl_req_info *req_info, struct genl_info *info) tb[ETHTOOL_A_COALESCE_TX_MAX_FRAMES_IRQ], &mod); ethnl_update_u32(&coalesce.stats_block_coalesce_usecs, tb[ETHTOOL_A_COALESCE_STATS_BLOCK_USECS], &mod); - ethnl_update_bool32(&coalesce.use_adaptive_rx_coalesce, - tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_RX], &mod); - ethnl_update_bool32(&coalesce.use_adaptive_tx_coalesce, - tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_TX], &mod); ethnl_update_u32(&coalesce.pkt_rate_low, tb[ETHTOOL_A_COALESCE_PKT_RATE_LOW], &mod); ethnl_update_u32(&coalesce.rx_coalesce_usecs_low, @@ -312,17 +310,25 @@ ethnl_set_coalesce(struct ethnl_req_info *req_info, struct genl_info *info) tb[ETHTOOL_A_COALESCE_TX_MAX_FRAMES_HIGH], &mod); ethnl_update_u32(&coalesce.rate_sample_interval, tb[ETHTOOL_A_COALESCE_RATE_SAMPLE_INTERVAL], &mod); - ethnl_update_u8(&kernel_coalesce.use_cqe_mode_tx, - tb[ETHTOOL_A_COALESCE_USE_CQE_MODE_TX], &mod); - ethnl_update_u8(&kernel_coalesce.use_cqe_mode_rx, - tb[ETHTOOL_A_COALESCE_USE_CQE_MODE_RX], &mod); ethnl_update_u32(&kernel_coalesce.tx_aggr_max_bytes, tb[ETHTOOL_A_COALESCE_TX_AGGR_MAX_BYTES], &mod); ethnl_update_u32(&kernel_coalesce.tx_aggr_max_frames, tb[ETHTOOL_A_COALESCE_TX_AGGR_MAX_FRAMES], &mod); ethnl_update_u32(&kernel_coalesce.tx_aggr_time_usecs, tb[ETHTOOL_A_COALESCE_TX_AGGR_TIME_USECS], &mod); - if (!mod) + + /* Update operation modes */ + ethnl_update_bool32(&coalesce.use_adaptive_rx_coalesce, + tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_RX], &mod_mode); + ethnl_update_bool32(&coalesce.use_adaptive_tx_coalesce, + tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_TX], &mod_mode); + ethnl_update_u8(&kernel_coalesce.use_cqe_mode_tx, + tb[ETHTOOL_A_COALESCE_USE_CQE_MODE_TX], &mod_mode); + ethnl_update_u8(&kernel_coalesce.use_cqe_mode_rx, + tb[ETHTOOL_A_COALESCE_USE_CQE_MODE_RX], &mod_mode); + + *dual_change = mod && mod_mode; + if (!mod && !mod_mode) return 0; ret = dev->ethtool_ops->set_coalesce(dev, &coalesce, &kernel_coalesce, @@ -330,6 +336,32 @@ ethnl_set_coalesce(struct ethnl_req_info *req_info, struct genl_info *info) return ret < 0 ? ret : 1; } +static int +ethnl_set_coalesce(struct ethnl_req_info *req_info, struct genl_info *info) +{ + bool dual_change; + int err, ret; + + /* SET_COALESCE may change operation mode and parameters in one call. + * Changing operation mode may cause the driver to reset the parameter + * values, and therefore ignore user input (driver does not know which + * parameters come from user and which are echoed back from ->get). + * To not complicate the drivers if user tries to change both the mode + * and parameters at once - call the driver twice. + */ + err = __ethnl_set_coalesce(req_info, info, &dual_change); + if (err < 0) + return err; + ret = err; + + if (ret && dual_change) { + err = __ethnl_set_coalesce(req_info, info, &dual_change); + if (err < 0) + return err; + } + return ret; +} + const struct ethnl_request_ops ethnl_coalesce_request_ops = { .request_cmd = ETHTOOL_MSG_COALESCE_GET, .reply_cmd = ETHTOOL_MSG_COALESCE_GET_REPLY, -- 2.7.4