[PATCH net-next] net: Adjust sk_gso_max_size once when set

[PATCH net-next] net: Adjust sk_gso_max_size once when set

[David Ahern]

sk_gso_max_size is set based on the dst dev. Both users of it
adjust the value by the same offset - (MAX_TCP_HEADER + 1). Rather
than compute the same adjusted value on each call do the adjustment
once when set.

Signed-off-by: David Ahern <dsahern@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
---
 net/core/sock.c       | 1 +
 net/ipv4/tcp.c        | 3 +--
 net/ipv4/tcp_output.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index e21485ab285d..114a6e220ba9 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2261,6 +2261,7 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
 			sk->sk_route_caps |= NETIF_F_SG | NETIF_F_HW_CSUM;
 			/* pairs with the WRITE_ONCE() in netif_set_gso_max_size() */
 			sk->sk_gso_max_size = READ_ONCE(dst->dev->gso_max_size);
+			sk->sk_gso_max_size -= (MAX_TCP_HEADER + 1);
 			/* pairs with the WRITE_ONCE() in netif_set_gso_max_segs() */
 			max_segs = max_t(u32, READ_ONCE(dst->dev->gso_max_segs), 1);
 		}
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3b75836db19b..1afa3f2f9a6d 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -893,8 +893,7 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
 		return mss_now;
 
 	/* Note : tcp_tso_autosize() will eventually split this later */
-	new_size_goal = sk->sk_gso_max_size - 1 - MAX_TCP_HEADER;
-	new_size_goal = tcp_bound_to_half_wnd(tp, new_size_goal);
+	new_size_goal = tcp_bound_to_half_wnd(tp, sk->sk_gso_max_size);
 
 	/* We try hard to avoid divides here */
 	size_goal = tp->gso_segs * mss_now;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5079832af5c1..11c06b9db801 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1960,7 +1960,7 @@ static u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
 
 	bytes = min_t(unsigned long,
 		      sk->sk_pacing_rate >> READ_ONCE(sk->sk_pacing_shift),
-		      sk->sk_gso_max_size - 1 - MAX_TCP_HEADER);
+		      sk->sk_gso_max_size);
 
 	/* Goal is to send at least one packet per ms,
 	 * not one big TSO packet every 100 ms.
-- 
2.25.1

Re: [PATCH net-next] net: Adjust sk_gso_max_size once when set

[Eric Dumazet]

On Mon, Jan 24, 2022 at 6:45 PM David Ahern <dsahern@kernel.org> wrote:
>
> sk_gso_max_size is set based on the dst dev. Both users of it
> adjust the value by the same offset - (MAX_TCP_HEADER + 1). Rather
> than compute the same adjusted value on each call do the adjustment
> once when set.
>
> Signed-off-by: David Ahern <dsahern@kernel.org>
> Cc: Eric Dumazet <edumazet@google.com>


SGTM, thanks.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next] net: Adjust sk_gso_max_size once when set

[David Ahern]

On 1/25/22 9:46 AM, Eric Dumazet wrote:
> On Mon, Jan 24, 2022 at 6:45 PM David Ahern <dsahern@kernel.org> wrote:
>>
>> sk_gso_max_size is set based on the dst dev. Both users of it
>> adjust the value by the same offset - (MAX_TCP_HEADER + 1). Rather
>> than compute the same adjusted value on each call do the adjustment
>> once when set.
>>
>> Signed-off-by: David Ahern <dsahern@kernel.org>
>> Cc: Eric Dumazet <edumazet@google.com>
> 
> 
> SGTM, thanks.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

The git history does not explain why MAX_TCP_HEADER is used to lower
sk_gso_max_size. Do you recall the history on it?

Re: [PATCH net-next] net: Adjust sk_gso_max_size once when set

[Eric Dumazet]

On Tue, Jan 25, 2022 at 9:16 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/25/22 9:46 AM, Eric Dumazet wrote:
> > On Mon, Jan 24, 2022 at 6:45 PM David Ahern <dsahern@kernel.org> wrote:
> >>
> >> sk_gso_max_size is set based on the dst dev. Both users of it
> >> adjust the value by the same offset - (MAX_TCP_HEADER + 1). Rather
> >> than compute the same adjusted value on each call do the adjustment
> >> once when set.
> >>
> >> Signed-off-by: David Ahern <dsahern@kernel.org>
> >> Cc: Eric Dumazet <edumazet@google.com>
> >
> >
> > SGTM, thanks.
> >
> > Reviewed-by: Eric Dumazet <edumazet@google.com>
>
> The git history does not explain why MAX_TCP_HEADER is used to lower
> sk_gso_max_size. Do you recall the history on it?

Simply that max IP datagram size is 64K

And TCP is sizing its payload size there (eg in  tcp_tso_autosize()),
when skb only contains payload.

Headers are added later in various xmit layers.

MAX_TCP_HEADER is chosen to avoid re-allocs of skb->head in typical workload.

Re: [PATCH net-next] net: Adjust sk_gso_max_size once when set

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 24 Jan 2022 19:45:11 -0700 you wrote:
> sk_gso_max_size is set based on the dst dev. Both users of it
> adjust the value by the same offset - (MAX_TCP_HEADER + 1). Rather
> than compute the same adjusted value on each call do the adjustment
> once when set.
> 
> Signed-off-by: David Ahern <dsahern@kernel.org>
> Cc: Eric Dumazet <edumazet@google.com>
> 
> [...]

Here is the summary with links:
  - [net-next] net: Adjust sk_gso_max_size once when set
    https://git.kernel.org/netdev/net-next/c/ab14f1802cfb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net: Adjust sk_gso_max_size once when set

[David Ahern]

On 1/25/22 10:20 AM, Eric Dumazet wrote:
>> The git history does not explain why MAX_TCP_HEADER is used to lower
>> sk_gso_max_size. Do you recall the history on it?
> 
> Simply that max IP datagram size is 64K
> 
> And TCP is sizing its payload size there (eg in  tcp_tso_autosize()),
> when skb only contains payload.
> 
> Headers are added later in various xmit layers.
> 
> MAX_TCP_HEADER is chosen to avoid re-allocs of skb->head in typical workload.

From what I can tell skb->head is allocated based on MAX_TCP_HEADER, and
payload is added as frags for TSO.

I was just curious because I noticed a few MTUs (I only looked multiples
of 100 from 1500 to 9000) can get an extra segment in a TSO packet and
stay under the 64kB limit if that offset had better information of the
actual header size needed (if any beyond network + tcp).

Re: [PATCH net-next] net: Adjust sk_gso_max_size once when set

[Eric Dumazet]

On Tue, Jan 25, 2022 at 3:49 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/25/22 10:20 AM, Eric Dumazet wrote:
> >> The git history does not explain why MAX_TCP_HEADER is used to lower
> >> sk_gso_max_size. Do you recall the history on it?
> >
> > Simply that max IP datagram size is 64K
> >
> > And TCP is sizing its payload size there (eg in  tcp_tso_autosize()),
> > when skb only contains payload.
> >
> > Headers are added later in various xmit layers.
> >
> > MAX_TCP_HEADER is chosen to avoid re-allocs of skb->head in typical workload.
>
> From what I can tell skb->head is allocated based on MAX_TCP_HEADER, and
> payload is added as frags for TSO.

Sure, but at the end, ip packet length field is 16bit wide, so
sizeof(network+tcp headers) + tcp_payload <= 65535

-> tcp_payload =< 65535 - sizeof(headers)

-> tcp_payload_max_per_skb = 65536 - ( MAX_TCP_HEADER + 1)

(This would not include Ethernet header)

>
> I was just curious because I noticed a few MTUs (I only looked multiples
> of 100 from 1500 to 9000) can get an extra segment in a TSO packet and
> stay under the 64kB limit if that offset had better information of the
> actual header size needed (if any beyond network + tcp).

TCP does not care about the extra sub-mss bytes that _could_ be added
to a TSO packet

So if I have 4K MTU (4096 bytes of payload), max TSO size would be 15*4k = 60K

Application writing 60*1024+100 bytes in one sendmsg() would send one
TSO packet of 15 segments, plus one extra tiny skb with 100 bytes of
payload.

I have played in the past trying to cover this case, but adding tests
in the fast path gave no noticeable difference for common workloads.