[Intel-wired-lan] [PATCH net v2] e1000e: Disable TSO on i219-LM card to increase speed

[Paul Menzel]

Dear Mateusz,


Thank you for the second iteration.

Am 21.12.22 um 14:25 schrieb Mateusz Palczewski:
> While using i219-LM card currently it was only possible to achieve
> about 60% of maximum speed due to regression introduced in Linux 5.8-rc1.
> This was caused by TSO not being disabled by default despite commit
> f29801030ac6 implementation. Fix that by moving the part of the code
> responsible for this outside of adapter->flags & FLAG_TSO_FORCE check.

Unfortunately, you missed to add all the information, that this can’t be 
reproduced in all situations, and you also forgot to add the datasheet 
section, where it’s written that TSO needs to be disabled.

> Fixes: f29801030ac6 ("e1000e: Disable TSO for buffer overrun workaround")

Why does

     if (!(adapter->flags & FLAG_TSO_FORCE)) {
     	[…]
     	if (hw->mac.type == e1000_pch_spt) {

not work in your situation? Is `FLAG_TSO_FORCE` set for network namespaces?

Anyway, I’d say, it’s the wrong commit to reference, as it does not 
introduce the performance regression you are seeing.

And now having dug in more into this change, I do not think, it can be 
accepted as is, as disabling TCP Segmentation Offload (TSO) would be a 
potential performance regression (CPU usage) on a lot of systems.

> Signed-off-by: Mateusz Palczewski <mateusz.palczewski at intel.com>
> ---
>   v2: Fixed commit message and comment inside the code
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 04acd1a992fa..2f5cf125ff77 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5307,10 +5307,6 @@ static void e1000_watchdog_task(struct work_struct *work)
>   					/* oops */
>   					break;
>   				}
> -				if (hw->mac.type == e1000_pch_spt) {
> -					netdev->features &= ~NETIF_F_TSO;
> -					netdev->features &= ~NETIF_F_TSO6;
> -				}
>   			}
>   
>   			/* enable transmits in the hardware, need to do this
> @@ -5326,6 +5322,14 @@ static void e1000_watchdog_task(struct work_struct *work)
>   			if (phy->ops.cfg_on_link_up)
>   				phy->ops.cfg_on_link_up(hw);
>   
> +			/* Disable TSO for i219 to avoid transfer speed
> +			 * being capped at 60%.
> +			 */

The results of the analysis would need to be added here, and the comment 
extended.

> +			if (hw->mac.type == e1000_pch_spt) {
> +					netdev->features &= ~NETIF_F_TSO;
> +					netdev->features &= ~NETIF_F_TSO6;
> +			}
> +
>   			netif_wake_queue(netdev);
>   			netif_carrier_on(netdev);
>   


Kind regards,

Paul