| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250716171834.0a9b430b@kernel.org>
Date: Wed, 16 Jul 2025 17:18:34 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>, "David S.
Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo
Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Mengyuan Lou
<mengyuanlou@...-swift.com>
Subject: Re: [PATCH net-next] net: wangxun: complete ethtool coalesce
options
On Mon, 14 Jul 2025 17:28:11 +0800 Jiawen Wu wrote:
> Support to use adaptive RX coalescing. Change the default RX coalesce
> usecs and limit the range of parameters for various types of devices,
> according to their hardware design.
Really seems like this should be split into multiple commits with
proper commit messages.
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> index d9de600e685a..cf7fc412a273 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> @@ -303,6 +303,9 @@ int wx_get_coalesce(struct net_device *netdev,
> else
> ec->rx_coalesce_usecs = wx->rx_itr_setting >> 2;
>
> + if (wx->rx_itr_setting == 1)
> + ec->use_adaptive_rx_coalesce = 1;
I don't understand the adaptive coalescing changes. You changed the
get/set functions but there seems to be no datapath code added actually
tuning the settings in this patch.
> /* if in mixed tx/rx queues per vector mode, report only rx settings */
> if (wx->q_vector[0]->tx.count && wx->q_vector[0]->rx.count)
> return 0;
> @@ -334,19 +337,28 @@ int wx_set_coalesce(struct net_device *netdev,
> return -EOPNOTSUPP;
> }
>
> - if (ec->tx_max_coalesced_frames_irq)
> - wx->tx_work_limit = ec->tx_max_coalesced_frames_irq;
> + if (ec->tx_max_coalesced_frames_irq > WX_MAX_TX_WORK ||
> + !ec->tx_max_coalesced_frames_irq)
> + return -EINVAL;
Looks unrelated to the adaptive settings
> + wx->tx_work_limit = ec->tx_max_coalesced_frames_irq;
>
> switch (wx->mac.type) {
> case wx_mac_sp:
> max_eitr = WX_SP_MAX_EITR;
> + rx_itr_param = WX_20K_ITR;
> + tx_itr_param = WX_12K_ITR;
> break;
> case wx_mac_aml:
> case wx_mac_aml40:
> max_eitr = WX_AML_MAX_EITR;
> + rx_itr_param = WX_20K_ITR;
> + tx_itr_param = WX_12K_ITR;
> break;
> default:
> max_eitr = WX_EM_MAX_EITR;
> + rx_itr_param = WX_7K_ITR;
> + tx_itr_param = WX_7K_ITR;
> break;
> }
>
> @@ -354,14 +366,26 @@ int wx_set_coalesce(struct net_device *netdev,
> (ec->tx_coalesce_usecs > (max_eitr >> 2)))
> return -EINVAL;
>
> + if (ec->use_adaptive_rx_coalesce) {
> + wx->rx_itr_setting = 1;
> + return 0;
> + }
> +
> + /* restore to default rx-usecs value when adaptive itr turn off */
> + /* user shall turn off adaptive itr and set user-defined rx usecs value
> + * in two cmds separately.
But why? Why do you have to overwrite the usec value that user passed
in from the user?
> + if (wx->rx_itr_setting == 1) {
> + wx->rx_itr_setting = rx_itr_param;
> + ec->rx_coalesce_usecs = wx->rx_itr_setting >> 2;
> + }
> +
> if (ec->rx_coalesce_usecs > 1)
> wx->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> else
> wx->rx_itr_setting = ec->rx_coalesce_usecs;
>
> - if (wx->rx_itr_setting == 1)
> - rx_itr_param = WX_20K_ITR;
> - else
> + if (wx->rx_itr_setting != 1)
> rx_itr_param = wx->rx_itr_setting;
>
> if (ec->tx_coalesce_usecs > 1)
> @@ -369,20 +393,8 @@ int wx_set_coalesce(struct net_device *netdev,
> else
> wx->tx_itr_setting = ec->tx_coalesce_usecs;
>
> - if (wx->tx_itr_setting == 1) {
> - switch (wx->mac.type) {
> - case wx_mac_sp:
> - case wx_mac_aml:
> - case wx_mac_aml40:
> - tx_itr_param = WX_12K_ITR;
> - break;
> - default:
> - tx_itr_param = WX_20K_ITR;
> - break;
> - }
> - } else {
> + if (wx->tx_itr_setting != 1)
> tx_itr_param = wx->tx_itr_setting;
> - }
>
> /* mixed Rx/Tx */
> if (wx->q_vector[0]->tx.count && wx->q_vector[0]->rx.count)
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index c363379126c0..b632e5f80ad5 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -411,6 +411,8 @@ enum WX_MSCA_CMD_value {
> #define WX_7K_ITR 595
> #define WX_12K_ITR 336
> #define WX_20K_ITR 200
> +#define WX_70K_ITR 57
> +#define WX_MAX_TX_WORK 65535
> #define WX_SP_MAX_EITR 0x00000FF8U
> #define WX_AML_MAX_EITR 0x00000FFFU
> #define WX_EM_MAX_EITR 0x00007FFCU
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> index 7e2d9ec38a30..2ca127a7aa77 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> @@ -115,7 +115,8 @@ static int ngbe_set_channels(struct net_device *dev,
>
> static const struct ethtool_ops ngbe_ethtool_ops = {
> .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> - ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
> + ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
> + ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
> .get_drvinfo = wx_get_drvinfo,
> .get_link = ethtool_op_get_link,
> .get_link_ksettings = wx_get_link_ksettings,
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index e0fc897b0a58..cb1b24a9ac6e 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -120,8 +120,8 @@ static int ngbe_sw_init(struct wx *wx)
> wx->rss_enabled = true;
>
> /* enable itr by default in dynamic mode */
> - wx->rx_itr_setting = 1;
> - wx->tx_itr_setting = 1;
> + wx->rx_itr_setting = WX_7K_ITR;
> + wx->tx_itr_setting = WX_7K_ITR;
Is the comment above these 2 lines stale now?
> /* set default ring sizes */
> wx->tx_ring_count = NGBE_DEFAULT_TXD;
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> index a4753402660e..86f3c106f1ed 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> @@ -538,7 +538,8 @@ static int txgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
>
> static const struct ethtool_ops txgbe_ethtool_ops = {
> .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> - ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
> + ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
> + ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
> .get_drvinfo = wx_get_drvinfo,
> .nway_reset = wx_nway_reset,
> .get_link = ethtool_op_get_link,
--
pw-bot: cr
Powered by blists - more mailing lists