[PATCH for-7.2] rtl8139: honor large send MSS value

[PATCH for-7.2] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a
Large-Send MSS value where the driver specifies the MSS. See the
datasheet here:
http://realtek.info/pdf/rtl8139cp.pdf

The code ignores this value and uses a hardcoded MSS of 1500 bytes
instead. When the MTU is less than 1500 bytes the hardcoded value
results in IP fragmentation and poor performance.

Use the Large-Send MSS value to correctly size Large-Send packets.

This issue was discussed in the past here:
https://lore.kernel.org/all/20161114162505.GD26664@stefanha-x1.localdomain/

Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
Reported-by: Tobias Fiebig <tobias+git@fiebig.nl>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1312
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/rtl8139.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Tobias: Please test this fix. Thanks!

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index e6643e3c9d..ecc4dcb07f 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -77,7 +77,6 @@
     ( ( input ) & ( size - 1 )  )
 
 #define ETHER_TYPE_LEN 2
-#define ETH_MTU     1500
 
 #define VLAN_TCI_LEN 2
 #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
@@ -2151,8 +2150,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
                 int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
 
-                DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d "
-                    "frame data %d specified MSS=%d\n", ETH_MTU,
+                DPRINTF("+++ C+ mode offloaded task TSO IP data %d "
+                    "frame data %d specified MSS=%d\n",
                     ip_data_len, saved_size - ETH_HLEN, large_send_mss);
 
                 int tcp_send_offset = 0;
@@ -2177,9 +2176,13 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                     goto skip_offload;
                 }
 
-                /* ETH_MTU = ip header len + tcp header len + payload */
+                /* MSS too small? */
+                if (tcp_hlen + hlen >= large_send_mss) {
+                    goto skip_offload;
+                }
+
                 int tcp_data_len = ip_data_len - tcp_hlen;
-                int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen;
+                int tcp_chunk_size = large_send_mss - hlen - tcp_hlen;
 
                 DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP "
                     "data len %d TCP chunk size %d\n", ip_data_len,
-- 
2.38.1

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
Just to keep you in the loop; Just applied the patch, but things didn't really get better; I am currently doing a 'make clean; make' for good measure (had built head first), and will also double-check that there is no accidental use of system-qemu libs. 

If that still doesn't show an effect, I'll hold tcpdump to the wire again.

With best regards,
Tobias 

-----Original Message-----
From: Stefan Hajnoczi <stefanha@redhat.com> 
Sent: Tuesday, 15 November 2022 17:37
To: qemu-devel@nongnu.org
Cc: jasowang@redhat.com; qemu-stable@nongnu.org; Stefan Hajnoczi <stefanha@redhat.com>; Russell King - ARM Linux <linux@armlinux.org.uk>; Tobias Fiebig <tobias+git@fiebig.nl>
Subject: [PATCH for-7.2] rtl8139: honor large send MSS value

The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a Large-Send MSS value where the driver specifies the MSS. See the datasheet here:
http://realtek.info/pdf/rtl8139cp.pdf

The code ignores this value and uses a hardcoded MSS of 1500 bytes instead. When the MTU is less than 1500 bytes the hardcoded value results in IP fragmentation and poor performance.

Use the Large-Send MSS value to correctly size Large-Send packets.

This issue was discussed in the past here:
https://lore.kernel.org/all/20161114162505.GD26664@stefanha-x1.localdomain/

Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
Reported-by: Tobias Fiebig <tobias+git@fiebig.nl>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1312
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/rtl8139.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Tobias: Please test this fix. Thanks!

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index e6643e3c9d..ecc4dcb07f 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -77,7 +77,6 @@
     ( ( input ) & ( size - 1 )  )
 
 #define ETHER_TYPE_LEN 2
-#define ETH_MTU     1500
 
 #define VLAN_TCI_LEN 2
 #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN) @@ -2151,8 +2150,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
                 int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
 
-                DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d "
-                    "frame data %d specified MSS=%d\n", ETH_MTU,
+                DPRINTF("+++ C+ mode offloaded task TSO IP data %d "
+                    "frame data %d specified MSS=%d\n",
                     ip_data_len, saved_size - ETH_HLEN, large_send_mss);
 
                 int tcp_send_offset = 0; @@ -2177,9 +2176,13 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                     goto skip_offload;
                 }
 
-                /* ETH_MTU = ip header len + tcp header len + payload */
+                /* MSS too small? */
+                if (tcp_hlen + hlen >= large_send_mss) {
+                    goto skip_offload;
+                }
+
                 int tcp_data_len = ip_data_len - tcp_hlen;
-                int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen;
+                int tcp_chunk_size = large_send_mss - hlen - tcp_hlen;
 
                 DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP "
                     "data len %d TCP chunk size %d\n", ip_data_len,
--
2.38.1

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Jason Wang]

On Wed, Nov 16, 2022 at 7:43 AM Tobias Fiebig <tobias@fiebig.nl> wrote:
>
> Heho,
> Just to keep you in the loop; Just applied the patch, but things didn't really get better; I am currently doing a 'make clean; make' for good measure (had built head first), and will also double-check that there is no accidental use of system-qemu libs.
>
> If that still doesn't show an effect, I'll hold tcpdump to the wire again.
>
> With best regards,
> Tobias

It might be also helpful to dump mss saw by Qemu to see if it really
changes or differs a lot from 1500.

Thanks

>
> -----Original Message-----
> From: Stefan Hajnoczi <stefanha@redhat.com>
> Sent: Tuesday, 15 November 2022 17:37
> To: qemu-devel@nongnu.org
> Cc: jasowang@redhat.com; qemu-stable@nongnu.org; Stefan Hajnoczi <stefanha@redhat.com>; Russell King - ARM Linux <linux@armlinux.org.uk>; Tobias Fiebig <tobias+git@fiebig.nl>
> Subject: [PATCH for-7.2] rtl8139: honor large send MSS value
>
> The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a Large-Send MSS value where the driver specifies the MSS. See the datasheet here:
> http://realtek.info/pdf/rtl8139cp.pdf
>
> The code ignores this value and uses a hardcoded MSS of 1500 bytes instead. When the MTU is less than 1500 bytes the hardcoded value results in IP fragmentation and poor performance.
>
> Use the Large-Send MSS value to correctly size Large-Send packets.
>
> This issue was discussed in the past here:
> https://lore.kernel.org/all/20161114162505.GD26664@stefanha-x1.localdomain/
>
> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
> Reported-by: Tobias Fiebig <tobias+git@fiebig.nl>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1312
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/net/rtl8139.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> Tobias: Please test this fix. Thanks!
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index e6643e3c9d..ecc4dcb07f 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -77,7 +77,6 @@
>      ( ( input ) & ( size - 1 )  )
>
>  #define ETHER_TYPE_LEN 2
> -#define ETH_MTU     1500
>
>  #define VLAN_TCI_LEN 2
>  #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN) @@ -2151,8 +2150,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>
>                  int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
>
> -                DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d "
> -                    "frame data %d specified MSS=%d\n", ETH_MTU,
> +                DPRINTF("+++ C+ mode offloaded task TSO IP data %d "
> +                    "frame data %d specified MSS=%d\n",
>                      ip_data_len, saved_size - ETH_HLEN, large_send_mss);
>
>                  int tcp_send_offset = 0; @@ -2177,9 +2176,13 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      goto skip_offload;
>                  }
>
> -                /* ETH_MTU = ip header len + tcp header len + payload */
> +                /* MSS too small? */
> +                if (tcp_hlen + hlen >= large_send_mss) {
> +                    goto skip_offload;
> +                }
> +
>                  int tcp_data_len = ip_data_len - tcp_hlen;
> -                int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen;
> +                int tcp_chunk_size = large_send_mss - hlen - tcp_hlen;
>
>                  DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP "
>                      "data len %d TCP chunk size %d\n", ip_data_len,
> --
> 2.38.1
>
>

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
I just tested around with the patch;
Good news: Certainly my builds are being executed. Also, if I patch the old code to have a MAX_MTU <= the max MTU on my path, throughput is ok.

Bad news: Something is wrong with getting the MSS in the patch you shared. When enabling DPRINT, values are off (sent MSS vs. printed MSS):
600 2060
800 2308
1000 2316
1023 2307
1200 3076
1400 3340 (most likely clamped to 1320)

Fiddling around a bit more, I found txdw0 printed earlier in the stack as hex (sent MSS, txdw0):
769 900502f5
1000 900503dc
1280 900504f4
1281 900504f5
1301 90050509
1317 90050519
1320 9005051c

This maps rather well to:
MSS = txdw0 - 2416246772 
MSS = txdw0 - 9004FFF4

Sadly, my C is 'non-existent' and it is kind-of 4AM, so also not in the brainspace to fill those gaps. But if one of you could look at the patch again, that would be nice. Otherwise, I should have some brainspace for this tomorrow night (UTC) again.

With best regards,
Tobias

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Jason Wang]

On Wed, Nov 16, 2022 at 10:59 AM Tobias Fiebig <tobias@fiebig.nl> wrote:
>
> Heho,
> I just tested around with the patch;
> Good news: Certainly my builds are being executed. Also, if I patch the old code to have a MAX_MTU <= the max MTU on my path, throughput is ok.
>
> Bad news: Something is wrong with getting the MSS in the patch you shared. When enabling DPRINT, values are off (sent MSS vs. printed MSS):
> 600 2060
> 800 2308
> 1000 2316
> 1023 2307
> 1200 3076
> 1400 3340 (most likely clamped to 1320)
>
> Fiddling around a bit more, I found txdw0 printed earlier in the stack as hex (sent MSS, txdw0):
> 769 900502f5
> 1000 900503dc
> 1280 900504f4
> 1281 900504f5
> 1301 90050509
> 1317 90050519
> 1320 9005051c
>
> This maps rather well to:
> MSS = txdw0 - 2416246772
> MSS = txdw0 - 9004FFF4
>
> Sadly, my C is 'non-existent' and it is kind-of 4AM, so also not in the brainspace to fill those gaps. But if one of you could look at the patch again, that would be nice. Otherwise, I should have some brainspace for this tomorrow night (UTC) again.
>

Ok, I think I found at least one issue:

/* large send MSS mask, bits 16...25 */
#define CP_TC_LGSEN_MSS_MASK ((1 << 12) - 1)

First, MSS occupies 11 bits from 16 to 26
Second, the mask is wrong it should be ((1 << 11) - 1)

Thanks

> With best regards,
> Tobias
>

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
> Ok, I think I found at least one issue:
> 
> /* large send MSS mask, bits 16...25 */
> #define CP_TC_LGSEN_MSS_MASK ((1 << 12) - 1)
> 
> First, MSS occupies 11 bits from 16 to 26 Second, the mask is wrong it should be ((1 << 11) - 1)
Awesome, thanks, will give this a shot later on and let you know.

With best regards,
Tobias

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
Quick follow-up; Applied the change you suggested, but there are still some things to test.

While this now works (mostly), MSS values are still off; Especially the behavior below <=1036 is difficult, as for v4 the minimum MTU is 576 and minimum MSS is 536:

Requested    DPRINT
1320              1292
1319              1291
1100              1024
1036              1024
1035                271
1000                268

So, I guess there is something else amiss; Will test more in-depth later tonight and shift the bits a bit; Also, I will look into behavior when forcing >1500 MTUs (in case that works with the rtl8139).

Sidenote: This all seems to be a non-issue for v6, as the RTL8139 does not support TSO for v6, so at least one thing less to worry about. ;-)

With best regards,
Tobias

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

I have sent a v2 with a fixed MSS mask constant but haven't tested it.

Thanks,
Stefan

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
Ok, I just learned more C than I ever wanted to. There is a bit more amiss here (ll from 7d7238c72b983cff5064734349d2d45be9c6282c):

In line 1916 of rtl8139.c we set txdw0; If we calculate the MSS at this point, it is consistently 12 below requested, but generally accurate. The bits that flip re: -12 must happen somewhere in the Linux kernel driver (ll 764 in drivers/net/ethernet/realtek/8139cp.c?); Didn't look there in-depth yet (and do not plan to, maybe one of you has more experience with this?) Given the consistency of this deviation, maybe just doing a +12 might be more straight forward.

However, in ll2030ff we reset a couple of status indicators. These overlap with the fields for the MSS, leading to inaccurate values being calculated later on; For example, requesting an MSS of 767 leads to an MSS of 3 being calculated by your patch; Similarly, requesting 1000 leads to 268. At least for the latter I see packets of that size being generated on the wire (which should also not happen, as the MSS should never be below 536; maybe a check could help here to make sure we are not trusting arbitrary values from the driver, esp. given the bobble of sec issues around PMTUD/MSS; Technically, now that MSS is defined earlier, we could also move this closer to the start of TSO large frame handling).

Below is also a draft patch following my suggestions (save txdw0, +12, check for <536) and some examples for what I described above, which I can on your last patch. Please note again that this is essentially the first time I do anything in C; Also, I wasn't sure what has less perf impact (save the whole 32bit of txdw0 even though it might not be needed vs. also doing the shift/& even though it might not be needed).

Apart from that, my patch seems to work, and the MSS gets set correctly; Someone else testing would be nice, though:

# MSS_requested=1320
RTL8139: +++ C+ mode offloaded task TSO IP data 2648 frame data 2668 specified MSS=1320

# MSS_requested=1000
RTL8139: +++ C+ mode offloaded task TSO IP data 2008 frame data 2028 specified MSS=1000

# MSS_requested=600
RTL8139: +++ C+ mode offloaded task TSO IP data 1796 frame data 1816 specified MSS=600

With best regards,
Tobias

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index e6643e3c9d..59321460b9 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -77,7 +77,6 @@
     ( ( input ) & ( size - 1 )  )
 
 #define ETHER_TYPE_LEN 2
-#define ETH_MTU     1500
 
 #define VLAN_TCI_LEN 2
 #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
@@ -1934,8 +1933,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 #define CP_TX_LS (1<<28)
 /* large send packet flag */
 #define CP_TX_LGSEN (1<<27)
-/* large send MSS mask, bits 16...25 */
-#define CP_TC_LGSEN_MSS_MASK ((1 << 12) - 1)
+/* large send MSS mask, bits 16...26 */
+#define CP_TC_LGSEN_MSS_SHIFT 16
+#define CP_TC_LGSEN_MSS_MASK ((1 << 11) - 1)
 
 /* IP checksum offload flag */
 #define CP_TX_IPCS (1<<18)
@@ -2027,6 +2027,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
             s->currCPlusTxDesc = 0;
     }
 
+    /* store unaltered txdw0 for later use in MSS calculation*/
+    uint32_t txdw0_save = txdw0;
+
     /* transfer ownership to target */
     txdw0 &= ~CP_TX_OWN;
 
@@ -2149,10 +2152,12 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                     goto skip_offload;
                 }
 
-                int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
+                /* set large_send_mss from txdw0 before overlapping mss fields were cleared */
+                int large_send_mss = ((txdw0_save >> CP_TC_LGSEN_MSS_SHIFT) &
+                    CP_TC_LGSEN_MSS_MASK) + 12;
 
-                DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d "
-                    "frame data %d specified MSS=%d\n", ETH_MTU,
+                DPRINTF("+++ C+ mode offloaded task TSO IP data %d "
+                    "frame data %d specified MSS=%d\n",
                     ip_data_len, saved_size - ETH_HLEN, large_send_mss);
 
                 int tcp_send_offset = 0;
@@ -2177,9 +2182,13 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
                     goto skip_offload;
                 }
 
-                /* ETH_MTU = ip header len + tcp header len + payload */
+                /* MSS too small? Min MSS = 536 */
+                if (tcp_hlen + hlen >= large_send_mss || 535 >= large_send_mss) {
+                    goto skip_offload;
+                }
+
                 int tcp_data_len = ip_data_len - tcp_hlen;
-                int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen;
+                int tcp_chunk_size = large_send_mss - hlen - tcp_hlen;
 
                 DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP "
                     "data len %d TCP chunk size %d\n", ip_data_len,



Some examples (with additional DPRINT capturing txdw0/MSS at various places; txdw0_0=ll1923, txdw0_4=ll2029, txdw0_5=ll2039, txdw0_cur=ll2153):

MSS_requested=556
+++ txdw0_cur=18000440 txdw0_cur_shift=1800 txdw0_cur_MSS=0;
+++ txdw0_0=9a200440 txdw0_0_shift=9a20 txdw0_0_MSS=544;
+++ txdw0_1=9a200440 txdw0_1_shift=9a20 txdw0_1_MSS=544;
+++ txdw0_2=9a200440 txdw0_2_shift=9a20 txdw0_2_MSS=544;
+++ txdw0_3=9a200440 txdw0_3_shift=9a20 txdw0_3_MSS=544;
+++ txdw0_4=9a200440 txdw0_4_shift=9a20 txdw0_4_MSS=544;
+++ txdw0_5=18000440 txdw0_5_shift=1800 txdw0_5_MSS=0;
+++ txdw0_6=18000440 txdw0_6_shift=1800 txdw0_6_MSS=0;
+++ txdw0_7=18000440 txdw0_7_shift=1800 txdw0_7_MSS=0;

MSS_requested=800
+++ txdw0_0=9b140cab txdw0_0_shift=9b14 txdw0_0_MSS=788;
+++ txdw0_1=9b140cab txdw0_1_shift=9b14 txdw0_1_MSS=788;
+++ txdw0_2=9b140cab txdw0_2_shift=9b14 txdw0_2_MSS=788;
+++ txdw0_3=9b140cab txdw0_3_shift=9b14 txdw0_3_MSS=788;
+++ txdw0_4=9b140cab txdw0_4_shift=9b14 txdw0_4_MSS=788;
+++ txdw0_5=19040cab txdw0_5_shift=1904 txdw0_5_MSS=260;
+++ txdw0_6=19040cab txdw0_6_shift=1904 txdw0_6_MSS=260;
+++ txdw0_7=19040cab txdw0_7_shift=1904 txdw0_7_MSS=260;

MSS_requested=1050
+++ txdw0_cur=1c0e07bf txdw0_cur_shift=1c0e txdw0_cur_MSS=1038;
+++ txdw0_0=9c0e07bf txdw0_0_shift=9c0e txdw0_0_MSS=1038;
+++ txdw0_1=9c0e07bf txdw0_1_shift=9c0e txdw0_1_MSS=1038;
+++ txdw0_2=9c0e07bf txdw0_2_shift=9c0e txdw0_2_MSS=1038;
+++ txdw0_3=9c0e07bf txdw0_3_shift=9c0e txdw0_3_MSS=1038;
+++ txdw0_4=9c0e07bf txdw0_4_shift=9c0e txdw0_4_MSS=1038;
+++ txdw0_5=1c0e07bf txdw0_5_shift=1c0e txdw0_5_MSS=1038;
+++ txdw0_6=1c0e07bf txdw0_6_shift=1c0e txdw0_6_MSS=1038;
+++ txdw0_7=1c0e07bf txdw0_7_shift=1c0e txdw0_7_MSS=1038;

MSS_requested=1060
+++ txdw0_cur=1c0809ff txdw0_cur_shift=1c08 txdw0_cur_MSS=1032;
+++ txdw0_0=9c1809ff txdw0_0_shift=9c18 txdw0_0_MSS=1048;
+++ txdw0_1=9c1809ff txdw0_1_shift=9c18 txdw0_1_MSS=1048;
+++ txdw0_2=9c1809ff txdw0_2_shift=9c18 txdw0_2_MSS=1048;
+++ txdw0_3=9c1809ff txdw0_3_shift=9c18 txdw0_3_MSS=1048;
+++ txdw0_4=9c1809ff txdw0_4_shift=9c18 txdw0_4_MSS=1048;
+++ txdw0_5=1c0809ff txdw0_5_shift=1c08 txdw0_5_MSS=1032;
+++ txdw0_6=1c0809ff txdw0_6_shift=1c08 txdw0_6_MSS=1032;
+++ txdw0_7=1c0809ff txdw0_7_shift=1c08 txdw0_7_MSS=1032;

MSS_requested=1320
+++ txdw0_cur=1d0c0b37 txdw0_cur_shift=1d0c txdw0_cur_MSS=1292;
+++ txdw0_0=9d1c0b37 txdw0_0_shift=9d1c txdw0_0_MSS=1308;
+++ txdw0_1=9d1c0b37 txdw0_1_shift=9d1c txdw0_1_MSS=1308;
+++ txdw0_2=9d1c0b37 txdw0_2_shift=9d1c txdw0_2_MSS=1308;
+++ txdw0_3=9d1c0b37 txdw0_3_shift=9d1c txdw0_3_MSS=1308;
+++ txdw0_4=9d1c0b37 txdw0_4_shift=9d1c txdw0_4_MSS=1308;
+++ txdw0_5=1d0c0b37 txdw0_5_shift=1d0c txdw0_5_MSS=1292;
+++ txdw0_6=1d0c0b37 txdw0_6_shift=1d0c txdw0_6_MSS=1292;
+++ txdw0_7=1d0c0b37 txdw0_7_shift=1d0c txdw0_7_MSS=1292;

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

On Wed, 16 Nov 2022 at 21:49, Tobias Fiebig <tobias@fiebig.nl> wrote:
>
> Heho,
> Ok, I just learned more C than I ever wanted to. There is a bit more amiss here (ll from 7d7238c72b983cff5064734349d2d45be9c6282c):
>
> In line 1916 of rtl8139.c we set txdw0; If we calculate the MSS at this point, it is consistently 12 below requested, but generally accurate. The bits that flip re: -12 must happen somewhere in the Linux kernel driver (ll 764 in drivers/net/ethernet/realtek/8139cp.c?); Didn't look there in-depth yet (and do not plan to, maybe one of you has more experience with this?) Given the consistency of this deviation, maybe just doing a +12 might be more straight forward.
>
> However, in ll2030ff we reset a couple of status indicators. These overlap with the fields for the MSS, leading to inaccurate values being calculated later on; For example, requesting an MSS of 767 leads to an MSS of 3 being calculated by your patch; Similarly, requesting 1000 leads to 268. At least for the latter I see packets of that size being generated on the wire (which should also not happen, as the MSS should never be below 536; maybe a check could help here to make sure we are not trusting arbitrary values from the driver, esp. given the bobble of sec issues around PMTUD/MSS; Technically, now that MSS is defined earlier, we could also move this closer to the start of TSO large frame handling).
>
> Below is also a draft patch following my suggestions (save txdw0, +12, check for <536) and some examples for what I described above, which I can on your last patch. Please note again that this is essentially the first time I do anything in C; Also, I wasn't sure what has less perf impact (save the whole 32bit of txdw0 even though it might not be needed vs. also doing the shift/& even though it might not be needed).
>
> Apart from that, my patch seems to work, and the MSS gets set correctly; Someone else testing would be nice, though:
>
> # MSS_requested=1320
> RTL8139: +++ C+ mode offloaded task TSO IP data 2648 frame data 2668 specified MSS=1320
>
> # MSS_requested=1000
> RTL8139: +++ C+ mode offloaded task TSO IP data 2008 frame data 2028 specified MSS=1000
>
> # MSS_requested=600
> RTL8139: +++ C+ mode offloaded task TSO IP data 1796 frame data 1816 specified MSS=600
>
> With best regards,
> Tobias
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index e6643e3c9d..59321460b9 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -77,7 +77,6 @@
>      ( ( input ) & ( size - 1 )  )
>
>  #define ETHER_TYPE_LEN 2
> -#define ETH_MTU     1500
>
>  #define VLAN_TCI_LEN 2
>  #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
> @@ -1934,8 +1933,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>  #define CP_TX_LS (1<<28)
>  /* large send packet flag */
>  #define CP_TX_LGSEN (1<<27)
> -/* large send MSS mask, bits 16...25 */
> -#define CP_TC_LGSEN_MSS_MASK ((1 << 12) - 1)
> +/* large send MSS mask, bits 16...26 */
> +#define CP_TC_LGSEN_MSS_SHIFT 16
> +#define CP_TC_LGSEN_MSS_MASK ((1 << 11) - 1)
>
>  /* IP checksum offload flag */
>  #define CP_TX_IPCS (1<<18)
> @@ -2027,6 +2027,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>              s->currCPlusTxDesc = 0;
>      }
>
> +    /* store unaltered txdw0 for later use in MSS calculation*/
> +    uint32_t txdw0_save = txdw0;
> +
>      /* transfer ownership to target */
>      txdw0 &= ~CP_TX_OWN;
>
> @@ -2149,10 +2152,12 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      goto skip_offload;
>                  }
>
> -                int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
> +                /* set large_send_mss from txdw0 before overlapping mss fields were cleared */
> +                int large_send_mss = ((txdw0_save >> CP_TC_LGSEN_MSS_SHIFT) &
> +                    CP_TC_LGSEN_MSS_MASK) + 12;

Hi Tobias,
Thanks for posting this information.

12 bytes hapens to be the size of the Ethernet header:
https://en.wikipedia.org/wiki/Ethernet_header#Structure

That could be a clue. I'll try to investigate some more.

Stefan

>
> -                DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d "
> -                    "frame data %d specified MSS=%d\n", ETH_MTU,
> +                DPRINTF("+++ C+ mode offloaded task TSO IP data %d "
> +                    "frame data %d specified MSS=%d\n",
>                      ip_data_len, saved_size - ETH_HLEN, large_send_mss);
>
>                  int tcp_send_offset = 0;
> @@ -2177,9 +2182,13 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      goto skip_offload;
>                  }
>
> -                /* ETH_MTU = ip header len + tcp header len + payload */
> +                /* MSS too small? Min MSS = 536 */
> +                if (tcp_hlen + hlen >= large_send_mss || 535 >= large_send_mss) {
> +                    goto skip_offload;
> +                }
> +
>                  int tcp_data_len = ip_data_len - tcp_hlen;
> -                int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen;
> +                int tcp_chunk_size = large_send_mss - hlen - tcp_hlen;
>
>                  DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP "
>                      "data len %d TCP chunk size %d\n", ip_data_len,
>
>
>
> Some examples (with additional DPRINT capturing txdw0/MSS at various places; txdw0_0=ll1923, txdw0_4=ll2029, txdw0_5=ll2039, txdw0_cur=ll2153):
>
> MSS_requested=556
> +++ txdw0_cur=18000440 txdw0_cur_shift=1800 txdw0_cur_MSS=0;
> +++ txdw0_0=9a200440 txdw0_0_shift=9a20 txdw0_0_MSS=544;
> +++ txdw0_1=9a200440 txdw0_1_shift=9a20 txdw0_1_MSS=544;
> +++ txdw0_2=9a200440 txdw0_2_shift=9a20 txdw0_2_MSS=544;
> +++ txdw0_3=9a200440 txdw0_3_shift=9a20 txdw0_3_MSS=544;
> +++ txdw0_4=9a200440 txdw0_4_shift=9a20 txdw0_4_MSS=544;
> +++ txdw0_5=18000440 txdw0_5_shift=1800 txdw0_5_MSS=0;
> +++ txdw0_6=18000440 txdw0_6_shift=1800 txdw0_6_MSS=0;
> +++ txdw0_7=18000440 txdw0_7_shift=1800 txdw0_7_MSS=0;
>
> MSS_requested=800
> +++ txdw0_0=9b140cab txdw0_0_shift=9b14 txdw0_0_MSS=788;
> +++ txdw0_1=9b140cab txdw0_1_shift=9b14 txdw0_1_MSS=788;
> +++ txdw0_2=9b140cab txdw0_2_shift=9b14 txdw0_2_MSS=788;
> +++ txdw0_3=9b140cab txdw0_3_shift=9b14 txdw0_3_MSS=788;
> +++ txdw0_4=9b140cab txdw0_4_shift=9b14 txdw0_4_MSS=788;
> +++ txdw0_5=19040cab txdw0_5_shift=1904 txdw0_5_MSS=260;
> +++ txdw0_6=19040cab txdw0_6_shift=1904 txdw0_6_MSS=260;
> +++ txdw0_7=19040cab txdw0_7_shift=1904 txdw0_7_MSS=260;
>
> MSS_requested=1050
> +++ txdw0_cur=1c0e07bf txdw0_cur_shift=1c0e txdw0_cur_MSS=1038;
> +++ txdw0_0=9c0e07bf txdw0_0_shift=9c0e txdw0_0_MSS=1038;
> +++ txdw0_1=9c0e07bf txdw0_1_shift=9c0e txdw0_1_MSS=1038;
> +++ txdw0_2=9c0e07bf txdw0_2_shift=9c0e txdw0_2_MSS=1038;
> +++ txdw0_3=9c0e07bf txdw0_3_shift=9c0e txdw0_3_MSS=1038;
> +++ txdw0_4=9c0e07bf txdw0_4_shift=9c0e txdw0_4_MSS=1038;
> +++ txdw0_5=1c0e07bf txdw0_5_shift=1c0e txdw0_5_MSS=1038;
> +++ txdw0_6=1c0e07bf txdw0_6_shift=1c0e txdw0_6_MSS=1038;
> +++ txdw0_7=1c0e07bf txdw0_7_shift=1c0e txdw0_7_MSS=1038;
>
> MSS_requested=1060
> +++ txdw0_cur=1c0809ff txdw0_cur_shift=1c08 txdw0_cur_MSS=1032;
> +++ txdw0_0=9c1809ff txdw0_0_shift=9c18 txdw0_0_MSS=1048;
> +++ txdw0_1=9c1809ff txdw0_1_shift=9c18 txdw0_1_MSS=1048;
> +++ txdw0_2=9c1809ff txdw0_2_shift=9c18 txdw0_2_MSS=1048;
> +++ txdw0_3=9c1809ff txdw0_3_shift=9c18 txdw0_3_MSS=1048;
> +++ txdw0_4=9c1809ff txdw0_4_shift=9c18 txdw0_4_MSS=1048;
> +++ txdw0_5=1c0809ff txdw0_5_shift=1c08 txdw0_5_MSS=1032;
> +++ txdw0_6=1c0809ff txdw0_6_shift=1c08 txdw0_6_MSS=1032;
> +++ txdw0_7=1c0809ff txdw0_7_shift=1c08 txdw0_7_MSS=1032;
>
> MSS_requested=1320
> +++ txdw0_cur=1d0c0b37 txdw0_cur_shift=1d0c txdw0_cur_MSS=1292;
> +++ txdw0_0=9d1c0b37 txdw0_0_shift=9d1c txdw0_0_MSS=1308;
> +++ txdw0_1=9d1c0b37 txdw0_1_shift=9d1c txdw0_1_MSS=1308;
> +++ txdw0_2=9d1c0b37 txdw0_2_shift=9d1c txdw0_2_MSS=1308;
> +++ txdw0_3=9d1c0b37 txdw0_3_shift=9d1c txdw0_3_MSS=1308;
> +++ txdw0_4=9d1c0b37 txdw0_4_shift=9d1c txdw0_4_MSS=1308;
> +++ txdw0_5=1d0c0b37 txdw0_5_shift=1d0c txdw0_5_MSS=1292;
> +++ txdw0_6=1d0c0b37 txdw0_6_shift=1d0c txdw0_6_MSS=1292;
> +++ txdw0_7=1d0c0b37 txdw0_7_shift=1d0c txdw0_7_MSS=1292;
>

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

After looking more closely at txdw0 it seems that the code mixes "Tx
command mode 0", "Tx command mode 1", and "Tx status mode". The bits
have different meanings in each mode, so this leads to confusion :).

Stefan

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
Ok, that explains a lot. I was also thinking that the vlan bit seem to overlap with the MTU field, and wanted to look at that later today.

Re the 12b: IIRC, the standard 1500 MTU for ethernet is already without the ethernet header; That can have up to 26b (18b basis, 4b 802.1q, 4b 802.1ad), but leads to a total frame length of 1526 (with other additions (MPLS) also just making the frame bigger, without touching the MTU/MSS). MSS than as usual -40 for v4 and ~ -60 for v6.

So I doubt that those 12b are subtracted for the ethernet header.

Does somebody still have an RTL8139 around, to test how the real hardware behaved?

With best regards,
Tobias

-----Original Message-----
From: Stefan Hajnoczi <stefanha@gmail.com> 
Sent: Thursday, 17 November 2022 12:16
To: Tobias Fiebig <tobias@fiebig.nl>
Cc: Jason Wang <jasowang@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; qemu-devel@nongnu.org; qemu-stable@nongnu.org; Russell King - ARM Linux <linux@armlinux.org.uk>
Subject: Re: [PATCH for-7.2] rtl8139: honor large send MSS value

After looking more closely at txdw0 it seems that the code mixes "Tx command mode 0", "Tx command mode 1", and "Tx status mode". The bits have different meanings in each mode, so this leads to confusion :).

Stefan

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

Hi Tobias,
My initial patch was broken. I did some cleanup and sent a v3.

Stefan

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
Thanks, will test the three patches later.

With best regards,
Tobias

-----Original Message-----
From: Stefan Hajnoczi <stefanha@gmail.com> 
Sent: Thursday, 17 November 2022 17:57
To: Tobias Fiebig <tobias@fiebig.nl>
Cc: Jason Wang <jasowang@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; qemu-devel@nongnu.org; qemu-stable@nongnu.org; Russell King - ARM Linux <linux@armlinux.org.uk>
Subject: Re: [PATCH for-7.2] rtl8139: honor large send MSS value

Hi Tobias,
My initial patch was broken. I did some cleanup and sent a v3.

Stefan

RE: [PATCH for-7.2] rtl8139: honor large send MSS value

[Tobias Fiebig]

Heho,
I gave v3 a shot and it performs as expected; For a requested MSS of 1320, TSO consistently uses a 1308 MSS. So for me, this patch works. Thanks for fixing this. :-)

Sadly, I do not have boxes to test with .1q around; If none of you has either, and that should be tested as well, I can give it a shot in the coming days, but it might take some time.

Side note: I found the 'missing' 12b in 12b of TCP options being added. :-| So sorry for that noise.

With best regards,
Tobias

MTU1500
RTL8139: +++ C+ mode offloaded task TSO IP data 7272 frame data 7292 specified MSS=1448
RTL8139: +++ C+ mode offloaded task TSO IP data 8720 frame data 8740 specified MSS=1448
RTL8139: +++ C+ mode offloaded task TSO IP data 8720 frame data 8740 specified MSS=1448
RTL8139: +++ C+ mode offloaded task TSO IP data 10168 frame data 10188 specified MSS=1448

MTU1320
RTL8139: +++ C+ mode offloaded task TSO IP data 2648 frame data 2668 specified MSS=1308
RTL8139: +++ C+ mode offloaded task TSO IP data 10496 frame data 10516 specified MSS=1308
RTL8139: +++ C+ mode offloaded task TSO IP data 6572 frame data 6592 specified MSS=1308
RTL8139: +++ C+ mode offloaded task TSO IP data 6572 frame data 6592 specified MSS=1308

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

On Thu, Nov 17, 2022, 15:42 Tobias Fiebig <tobias@fiebig.nl> wrote:

> Heho,
> I gave v3 a shot and it performs as expected; For a requested MSS of 1320,
> TSO consistently uses a 1308 MSS. So for me, this patch works. Thanks for
> fixing this. :-)
>
> Sadly, I do not have boxes to test with .1q around; If none of you has
> either, and that should be tested as well, I can give it a shot in the
> coming days, but it might take some time.
>

Awesome, thanks for your help! I don't have a .1q setup on hand.

Stefan


> Side note: I found the 'missing' 12b in 12b of TCP options being added.
> :-| So sorry for that noise.
>
> With best regards,
> Tobias
>
> MTU1500
> RTL8139: +++ C+ mode offloaded task TSO IP data 7272 frame data 7292
> specified MSS=1448
> RTL8139: +++ C+ mode offloaded task TSO IP data 8720 frame data 8740
> specified MSS=1448
> RTL8139: +++ C+ mode offloaded task TSO IP data 8720 frame data 8740
> specified MSS=1448
> RTL8139: +++ C+ mode offloaded task TSO IP data 10168 frame data 10188
> specified MSS=1448
>
> MTU1320
> RTL8139: +++ C+ mode offloaded task TSO IP data 2648 frame data 2668
> specified MSS=1308
> RTL8139: +++ C+ mode offloaded task TSO IP data 10496 frame data 10516
> specified MSS=1308
> RTL8139: +++ C+ mode offloaded task TSO IP data 6572 frame data 6592
> specified MSS=1308
> RTL8139: +++ C+ mode offloaded task TSO IP data 6572 frame data 6592
> specified MSS=1308
>
>

[-- Attachment #2: Type: text/html, Size: 2039 bytes --]

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Jason Wang]

在 2022/11/17 19:26, Tobias Fiebig 写道:
> Heho,
> Ok, that explains a lot. I was also thinking that the vlan bit seem to overlap with the MTU field, and wanted to look at that later today.
>
> Re the 12b: IIRC, the standard 1500 MTU for ethernet is already without the ethernet header; That can have up to 26b (18b basis, 4b 802.1q, 4b 802.1ad), but leads to a total frame length of 1526 (with other additions (MPLS) also just making the frame bigger, without touching the MTU/MSS). MSS than as usual -40 for v4 and ~ -60 for v6.
>
> So I doubt that those 12b are subtracted for the ethernet header.
>
> Does somebody still have an RTL8139 around, to test how the real hardware behaved?


This would be very hard, I can think that the Qemu rtl8139 emulation is 
probably the only user for kernel 8139cp drivers for years.

Thanks


>
> With best regards,
> Tobias
>
> -----Original Message-----
> From: Stefan Hajnoczi <stefanha@gmail.com>
> Sent: Thursday, 17 November 2022 12:16
> To: Tobias Fiebig <tobias@fiebig.nl>
> Cc: Jason Wang <jasowang@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; qemu-devel@nongnu.org; qemu-stable@nongnu.org; Russell King - ARM Linux <linux@armlinux.org.uk>
> Subject: Re: [PATCH for-7.2] rtl8139: honor large send MSS value
>
> After looking more closely at txdw0 it seems that the code mixes "Tx command mode 0", "Tx command mode 1", and "Tx status mode". The bits have different meanings in each mode, so this leads to confusion :).
>
> Stefan
>

Re: [PATCH for-7.2] rtl8139: honor large send MSS value

[Jason Wang]

On Wed, Nov 16, 2022 at 12:37 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a
> Large-Send MSS value where the driver specifies the MSS. See the
> datasheet here:
> http://realtek.info/pdf/rtl8139cp.pdf
>
> The code ignores this value and uses a hardcoded MSS of 1500 bytes
> instead. When the MTU is less than 1500 bytes the hardcoded value
> results in IP fragmentation and poor performance.
>
> Use the Large-Send MSS value to correctly size Large-Send packets.
>
> This issue was discussed in the past here:
> https://lore.kernel.org/all/20161114162505.GD26664@stefanha-x1.localdomain/
>
> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
> Reported-by: Tobias Fiebig <tobias+git@fiebig.nl>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1312
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  hw/net/rtl8139.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> Tobias: Please test this fix. Thanks!
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index e6643e3c9d..ecc4dcb07f 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -77,7 +77,6 @@
>      ( ( input ) & ( size - 1 )  )
>
>  #define ETHER_TYPE_LEN 2
> -#define ETH_MTU     1500
>
>  #define VLAN_TCI_LEN 2
>  #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
> @@ -2151,8 +2150,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>
>                  int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
>
> -                DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d "
> -                    "frame data %d specified MSS=%d\n", ETH_MTU,
> +                DPRINTF("+++ C+ mode offloaded task TSO IP data %d "
> +                    "frame data %d specified MSS=%d\n",
>                      ip_data_len, saved_size - ETH_HLEN, large_send_mss);
>
>                  int tcp_send_offset = 0;
> @@ -2177,9 +2176,13 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      goto skip_offload;
>                  }
>
> -                /* ETH_MTU = ip header len + tcp header len + payload */
> +                /* MSS too small? */
> +                if (tcp_hlen + hlen >= large_send_mss) {
> +                    goto skip_offload;
> +                }
> +
>                  int tcp_data_len = ip_data_len - tcp_hlen;
> -                int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen;
> +                int tcp_chunk_size = large_send_mss - hlen - tcp_hlen;
>
>                  DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP "
>                      "data len %d TCP chunk size %d\n", ip_data_len,
> --
> 2.38.1
>