1. Patchew
  2. QEMU
  3. View series

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

Tobias Fiebig posted 1 patch 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/01e701d8fa2f$4124d750$c36e85f0$@fiebig.nl
Maintainers: Jason Wang <jasowang@redhat.com>
RE: [PATCH for-7.2] rtl8139: honor large send MSS value
Posted by Tobias Fiebig 1 year, 5 months ago 
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
Posted by Stefan Hajnoczi 1 year, 5 months ago 
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
Posted by Stefan Hajnoczi 1 year, 5 months ago 
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
Posted by Tobias Fiebig 1 year, 5 months ago 
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
Posted by Jason Wang 1 year, 5 months ago 
在 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
Posted by Stefan Hajnoczi 1 year, 5 months ago 
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
Posted by Tobias Fiebig 1 year, 5 months ago 
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
Posted by Stefan Hajnoczi 1 year, 5 months ago 
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
>
>
RE: [PATCH for-7.2] rtl8139: honor large send MSS value
Posted by Tobias Fiebig 1 year, 5 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.