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

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

[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.

Fixes: f29801030ac6 ("e1000e: Disable TSO for buffer overrun workaround")
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@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%.
+			 */
+			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);
 
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [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@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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Palczewski, Mateusz]

Hi,

sorry for late respond but I needed to create new setup to be sure that this issue is not related to namespaces.

>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?
>

I have tested this on new setup and without this patch I still see the speed being aroung only ~690 Mbits/sec and after applying it stable 934 Mbits/sec

To give more information on the setup - I have tested it with i219-LM that is built into server with Intel Skylake chip. The results are the same for connection using namespaces in the same server as well as having it connected to another server.
>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.
>
Can You please elaborate more on that? 
>> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@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
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Paul Menzel]

Dear Mateusz,


Am 28.12.22 um 11:43 schrieb Palczewski, Mateusz:

> sorry for late respond but I needed to create new setup to be sure
> that this issue is not related to namespaces.

No problem. Thank you for your reply, and sorry for my late response due
to the holidays.

>> 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?
>  > I have tested this on new setup and without this patch I still see
> the speed being aroung only ~690 Mbits/sec and after applying it
> stable 934 Mbits/sec

Please provide the instructions how to set up the network namespace, 
that I can test that.

> To give more information on the setup - I have tested it with i219-LM
> that is built into server with Intel Skylake chip. The results are
> the same for connection using namespaces in the same server as well
> as having it connected to another server.

I only tested this on desktops. Please give more details on the actual 
device (`lspci -nn`).

Please figure out, with a debugger, tracing or print statements, why the 
condition above is not reached in your setup.

>> 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.
>> 
> Can You please elaborate more on that?

If you disable TSO the “calculations“ are done on the CPU instead of the 
network device, so less compute resources are available for computing. 
As it works fine on the systems I tested with, it would be a performance 
regression, on the systems, where your problem does not occur.


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Palczewski, Mateusz]

Hello 

>
>Dear Mateusz,
>
>
>Am 28.12.22 um 11:43 schrieb Palczewski, Mateusz:
>
>> sorry for late respond but I needed to create new setup to be sure 
>> that this issue is not related to namespaces.
>
>No problem. Thank you for your reply, and sorry for my late response due to the holidays.
>
>>> 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?
>>  > I have tested this on new setup and without this patch I still see 
>> the speed being aroung only ~690 Mbits/sec and after applying it 
>> stable 934 Mbits/sec
>
>Please provide the instructions how to set up the network namespace, that I can test that.
 
 In my setup i219 interface is called eno1 and it's connected Back2Back with other card inside same server ens1f3.
 I have created new namespace using "ip netns add test", then I have added my i219 interface using  " ip link set eno1 netns test",
 bring it up using "ip netns exec test ifconfig eno1 up", added ip addresses to both of the interfaces
 "ip netns exec test ip addr add 10.1.1.1/24 dev eno1" on namespace interface and "ip addr add 10.1.1.2/24 dev ens1f3".
 Then on one terminal I ran "ip netns exec test iperf3 -c 10.1.1.2" and on the second one "iperf3 -s". This resulted on the speed being capped
 at ~690Mbits/s.
 
But after Your comments I have changed my setup to have i219 connected to another server to avoid using namespaces. The results are that the TSO seems to be  disabled by default right after the system boots with e1000e driver, but after reloading it with rmmod and modprobe it seems to be disabled by default. I will dig more into it. But please tell me if You observe the same thing. 

>
>> To give more information on the setup - I have tested it with i219-LM 
>> that is built into server with Intel Skylake chip. The results are the 
>> same for connection using namespaces in the same server as well as 
>> having it connected to another server.
>
>I only tested this on desktops. Please give more details on the actual device (`lspci -nn`).
 Here is my output

 # lspci -nn | grep "Eth"
 00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet Connection (3) I219-LM [8086:15b9] (rev 09)

>
>Please figure out, with a debugger, tracing or print statements, why the condition above is not reached in your setup.

 I will try to test more if maybe this issue is present only after reloading the driver instead of namespaces themselves. 

>
>>> 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.
>>> 
>> Can You please elaborate more on that?
>
>If you disable TSO the “calculations“ are done on the CPU instead of the network device, so less compute resources are available for computing. 
>As it works fine on the systems I tested with, it would be a performance regression, on the systems, where your problem does not occur.
>
>
>Kind regards,
>
>Paul
>

Regards,
Mateusz
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Paul Menzel]

Dear Mateusz,


Am 03.01.23 um 16:15 schrieb Palczewski, Mateusz:

>> Am 28.12.22 um 11:43 schrieb Palczewski, Mateusz:

>>>> 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?
>>>   > I have tested this on new setup and without this patch I still see
>>> the speed being aroung only ~690 Mbits/sec and after applying it
>>> stable 934 Mbits/sec
>>
>> Please provide the instructions how to set up the network namespace, that I can test that.
> 
> In my setup i219 interface is called eno1 and it's connected
> Back2Back with other card inside same server ens1f3. I have created
> new namespace using "ip netns add test", then I have added my i219
> interface using  " ip link set eno1 netns test", bring it up using
> "ip netns exec test ifconfig eno1 up", added ip addresses to both of
> the interfaces "ip netns exec test ip addr add 10.1.1.1/24 dev eno1"
> on namespace interface and "ip addr add 10.1.1.2/24 dev ens1f3". Then
> on one terminal I ran "ip netns exec test iperf3 -c 10.1.1.2" and on
> the second one "iperf3 -s". This resulted on the speed being capped 
> at ~690Mbits/s.

Thank you. (Though, I do not fully understand why you need a network
namespace to send traffic between two cards even in the same system.)

> But after Your comments I have changed my setup to have i219
> connected to another server to avoid using namespaces. The results
> are that the TSO seems to be  disabled by default right after the
> system boots with e1000e driver, but after reloading it with rmmod
> and modprobe it seems to be disabled by default. I will dig more into
> it. But please tell me if You observe the same thing.

Very interesting. Discussing more about the issue, I now see that the 
commit message summary confused me, and I assumed, TSO was enabled. 
After the discussion, I checked with ethtool to find out, that it’s off 
on the systems I tested with.

     $ ethtool -k net00 | grep tcp-segmentation-offload
     tcp-segmentation-offload: off

Turning it on with `sudo ethtool -K net00 tso on` indeed decreased the 
bandwidth from 846 Mbits/sec to 507 Mbits/sec between two Dell Precision 
3620 connected over an office switch.

     00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet 
Connection (2) I219-LM [8086:15b7] (rev 31)

Removing and loading the module *e1000e* does not turn TSO on though here.

>>> To give more information on the setup - I have tested it with i219-LM
>>> that is built into server with Intel Skylake chip. The results are the
>>> same for connection using namespaces in the same server as well as
>>> having it connected to another server.
>>
>> I only tested this on desktops. Please give more details on the actual device (`lspci -nn`).
> Here is my output
> 
>   # lspci -nn | grep "Eth"
>   00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet Connection (3) I219-LM [8086:15b9] (rev 09)

(I would have hoped Intel to have fixed the TSO issue in newer 
revisions. Do you know if that problem is known to the hardware guys.)

>> Please figure out, with a debugger, tracing or print statements,
>> why the condition above is not reached in your setup.
> 
> I will try to test more if maybe this issue is present only after
> reloading the driver instead of namespaces themselves.

Good luck figuring it out.

>>>> 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.
>>>>
>>> Can You please elaborate more on that?
>>
>> If you disable TSO the “calculations“ are done on the CPU instead
>> of the network device, so less compute resources are available for
>> computing. As it works fine on the systems I tested with, it would be
>> a performance regression, on the systems, where your problem does not
>> occur.

As TSO is off on my systems, my point is mood.


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Tony Nguyen]

On 12/21/2022 5:25 AM, Mateusz Palczewski wrote:
> 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.

Have you looked into changing the initial netdev features to prevent 
this? Seems like it would be more effective to have it off fixed for 
this MAC over turning it off in the watchdog every time.

> 
> Fixes: f29801030ac6 ("e1000e: Disable TSO for buffer overrun workaround")
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@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%.
> +			 */
> +			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);
>   
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan