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

[Jason Wang]

On Fri, Nov 18, 2022 at 12:56 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.
>
> Jason Wang <jasowang@redhat.com> noticed that the Large-Send MSS value
> mask was incorrect so it is adjusted to match the datasheet and Linux
> 8139cp driver.
>
> 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 | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 6dd7a8e6e0..700b1b66b6 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)
> @@ -2152,10 +2152,11 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      goto skip_offload;
>                  }
>
> -                int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
> +                int large_send_mss = (txdw0 >> CP_TC_LGSEN_MSS_SHIFT) &
> +                                     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;
> @@ -2180,25 +2181,22 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      goto skip_offload;
>                  }
>
> -                /* ETH_MTU = ip header len + tcp header len + payload */
>                  int tcp_data_len = ip_data_len - tcp_hlen;
> -                int tcp_chunk_size = ETH_MTU - 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,
> -                    tcp_hlen, tcp_data_len, tcp_chunk_size);
> +                    "data len %d\n", ip_data_len, tcp_hlen, tcp_data_len);
>
>                  /* note the cycle below overwrites IP header data,
>                     but restores it from saved_ip_header before sending 
> packet */
>
>                  int is_last_frame = 0;
>
> -                for (tcp_send_offset = 0; tcp_send_offset < tcp_data_len; 
> tcp_send_offset += tcp_chunk_size)
> +                for (tcp_send_offset = 0; tcp_send_offset < tcp_data_len; 
> tcp_send_offset += large_send_mss)
>                  {
> -                    uint16_t chunk_size = tcp_chunk_size;
> +                    uint16_t chunk_size = large_send_mss;
>
>                      /* check if this is the last frame */
> -                    if (tcp_send_offset + tcp_chunk_size >= tcp_data_len)
> +                    if (tcp_send_offset + large_send_mss >= tcp_data_len)
>                      {
>                          is_last_frame = 1;
>                          chunk_size = tcp_data_len - tcp_send_offset;
> @@ -2247,7 +2245,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>                      ip->ip_len = cpu_to_be16(hlen + tcp_hlen + chunk_size);
>
>                      /* increment IP id for subsequent frames */
> -                    ip->ip_id = cpu_to_be16(tcp_send_offset/tcp_chunk_size + 
> be16_to_cpu(ip->ip_id));
> +                    ip->ip_id = cpu_to_be16(tcp_send_offset/large_send_mss + 
> be16_to_cpu(ip->ip_id));
>
>                      ip->ip_sum = 0;
>                      ip->ip_sum = ip_checksum(eth_payload_data, hlen);
> --
> 2.38.1
>