| 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: <178679d2-938f-3d7d-f03c-2c0210288099@amd.com>
Date: Tue, 1 Aug 2023 11:18:12 -0700
From: Brett Creeley <bcreeley@....com>
To: Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
netdev@...r.kernel.org
Cc: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>,
sasha.neftin@...el.com, Naama Meir <naamax.meir@...ux.intel.com>,
Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net v2 2/2] igc: Modify the tx-usecs coalesce setting
On 8/1/2023 10:27 AM, Tony Nguyen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>
>
> This patch enables users to modify the tx-usecs parameter.
> The rx-usecs value will adhere to the same value as tx-usecs
> if the queue pair setting is enabled.
>
> How to test:
> User can set the coalesce value using ethtool command.
>
> Example command:
> Set: ethtool -C <interface>
>
> Previous output:
>
> root@...DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10
> netlink error: Invalid argument
>
> New output:
>
> root@...DYHUSAINI:~# ethtool -C enp170s0 tx-usecs 10
> rx-usecs: 10
> rx-frames: n/a
> rx-usecs-irq: n/a
> rx-frames-irq: n/a
>
> tx-usecs: 10
> tx-frames: n/a
> tx-usecs-irq: n/a
> tx-frames-irq: n/a
>
> Fixes: 8c5ad0dae93c ("igc: Add ethtool support")
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>
> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---
> drivers/net/ethernet/intel/igc/igc_ethtool.c | 49 +++++++++++++++-----
> 1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 62d925b26f2c..ed67d9061452 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -914,19 +914,44 @@ static int igc_ethtool_set_coalesce(struct net_device *netdev,
> adapter->flags &= ~IGC_FLAG_DMAC;
> }
>
> - /* convert to rate of irq's per second */
> - if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3)
> - adapter->rx_itr_setting = ec->rx_coalesce_usecs;
> - else
> - adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
> + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) {
> + u32 old_tx_itr, old_rx_itr;
> +
> + /* This is to get back the original value before byte shifting */
> + old_tx_itr = (adapter->tx_itr_setting <= 3) ?
> + adapter->tx_itr_setting : adapter->tx_itr_setting >> 2;
> +
> + old_rx_itr = (adapter->rx_itr_setting <= 3) ?
> + adapter->rx_itr_setting : adapter->rx_itr_setting >> 2;
> +
> + if (old_tx_itr != ec->tx_coalesce_usecs) {
> + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3)
> + adapter->tx_itr_setting = ec->tx_coalesce_usecs;
> + else
> + adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2;
It seems like this if/else flow is duplicated multiple times throughout
this patch. Maybe it would be useful to introduce a helper function so
you can just do the following:
adapter->tx_itr_setting =
igc_ethtool_coalesce_to_itr_setting(ec->tx_coalesce);
> +
> + adapter->rx_itr_setting = adapter->tx_itr_setting;
> + } else if (old_rx_itr != ec->rx_coalesce_usecs) {
> + if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3)
> + adapter->rx_itr_setting = ec->rx_coalesce_usecs;
> + else
> + adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
Seems like the helper function could work for both tx/rx:
adapter->rx_itr_setting =
igc_ethtool_coalsece_to_itr_setting(ec->rx_coalesce);
> +
> + adapter->tx_itr_setting = adapter->rx_itr_setting;
> + }
> + } else {
> + /* convert to rate of irq's per second */
> + if (ec->rx_coalesce_usecs && ec->rx_coalesce_usecs <= 3)
> + adapter->rx_itr_setting = ec->rx_coalesce_usecs;
> + else
> + adapter->rx_itr_setting = ec->rx_coalesce_usecs << 2;
adapter->rx_itr_setting =
igc_ethtool_coalsece_to_itr_setting(ec->rx_coalesce);
>
> - /* convert to rate of irq's per second */
> - if (adapter->flags & IGC_FLAG_QUEUE_PAIRS)
> - adapter->tx_itr_setting = adapter->rx_itr_setting;
> - else if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3)
> - adapter->tx_itr_setting = ec->tx_coalesce_usecs;
> - else
> - adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2;
> + /* convert to rate of irq's per second */
> + if (ec->tx_coalesce_usecs && ec->tx_coalesce_usecs <= 3)
> + adapter->tx_itr_setting = ec->tx_coalesce_usecs;
> + else
> + adapter->tx_itr_setting = ec->tx_coalesce_usecs << 2;
adapter->tx_itr_setting =
igc_ethtool_coalesce_to_itr_setting(ec->tx_coalesce);
> + }
>
> for (i = 0; i < adapter->num_q_vectors; i++) {
> struct igc_q_vector *q_vector = adapter->q_vector[i];
> --
> 2.38.1
>
>
Powered by blists - more mailing lists