[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-7.2] rtl8139: honor large send MSS value
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH for-7.2] rtl8139: honor large send MSS value |
Date: |
Thu, 17 Nov 2022 06:07:32 -0500 |
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;
>
- [PATCH for-7.2] rtl8139: honor large send MSS value, Stefan Hajnoczi, 2022/11/15
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/15
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/15
- Re: [PATCH for-7.2] rtl8139: honor large send MSS value, Jason Wang, 2022/11/16
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/16
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/16
- Re: [PATCH for-7.2] rtl8139: honor large send MSS value, Stefan Hajnoczi, 2022/11/16
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/16
- Re: [PATCH for-7.2] rtl8139: honor large send MSS value,
Stefan Hajnoczi <=
- Re: [PATCH for-7.2] rtl8139: honor large send MSS value, Stefan Hajnoczi, 2022/11/17
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/17
- Re: [PATCH for-7.2] rtl8139: honor large send MSS value, Stefan Hajnoczi, 2022/11/17
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/17
- RE: [PATCH for-7.2] rtl8139: honor large send MSS value, Tobias Fiebig, 2022/11/17
- Re: [PATCH for-7.2] rtl8139: honor large send MSS value, Stefan Hajnoczi, 2022/11/17
- Re: [PATCH for-7.2] rtl8139: honor large send MSS value, Jason Wang, 2022/11/18
Re: [PATCH for-7.2] rtl8139: honor large send MSS value, Jason Wang, 2022/11/15