Re: [PATCH] tcp: make urg+gso work for real this time
From: Ilpo Järvinen <hidden>
Date: 2008-12-03 18:31:11
On Wed, 3 Dec 2008, Petr Tesarik wrote:
Ilpo Järvinen píse v St 03. 12. 2008 v 13:22 +0200:quoted
I should have noticed this earlier... :-) The previous solution to URG+GSO/TSO will cause SACK block tcp_fragment to do zig-zig patterns, or even worse, a steep downward slope into packet counting because each skb pcount would be truncated to pcount of 2 and then the following fragments of the later portion would restore the window again.Oops, not nice. :(quoted
Basically this reverts "tcp: Do not use TSO/GSO when there is urgent data" (33cf71cee1). It also removes some unnecessary code from tcp_current_mss that didn't work as intented either (could be that something was changed down the road, or it might have been broken since the dawn of time) because it only works once urg is already written while this bug shows up starting from ~64k before the urg point. The retransmissions already are split to mss sized chunks, so only new data sending paths need splitting in case they have a segment otherwise suitable for gso/tso. The actually check can be improved to be more narrow but since this is late -rc already, I'll postpone thinking the more fine-grained things. Signed-off-by: Ilpo Järvinen <redacted> Cc: Petr Tesarik <redacted>Yes, re-fragmenting the packet here is probably the best way to go. But repeating the same condition on multiple places is not so nice. What about this little improvement?
Yeah, this too will work... Though not of too much significance since I've already most of unification of those two functions in a local queue awaiting testing, polish up and submit to net-next. When sending an updated version davem prefers to have full changelog in it btw, it saves some work for him.
(I didn't roll the revert of 33cf71cee1 into it, as it makes the actual change less obvious. I think there are better ways of doing a revert in git.)
Whatever, I've no strong opinion either way though it's just broken with and without the revert anyway, the full thing is needed to actually fix it.
quoted hunk ↗ jump to hunk
Signed-off-by: Petr Tesarik <redacted> Cc: Ilpo Järvinen <redacted> -- tcp_output.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index ba85d88..a45c62d 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c@@ -1028,10 +1028,6 @@ unsigned int tcp_sync_mss(struct sock *sk, u32 pmtu) /* Compute the current effective MSS, taking SACKs and IP options, * and even PMTU discovery events into account. - * - * LARGESEND note: !tcp_urg_mode is overkill, only frames up to snd_up - * cannot be large. However, taking into account rare use of URG, this - * is not a big flaw. */ unsigned int tcp_current_mss(struct sock *sk, int large_allowed) {@@ -1046,7 +1042,7 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed) mss_now = tp->mss_cache; - if (large_allowed && sk_can_gso(sk) && !tcp_urg_mode(tp)) + if (large_allowed && sk_can_gso(sk)) doing_tso = 1; if (dst) {@@ -1113,6 +1109,10 @@ static void tcp_cwnd_validate(struct sock *sk) * return whenever allowed by the other factors). Basically we need the * modulo only when the receiver window alone is the limiting factor or * when we would be allowed to send the split-due-to-Nagle skb fully. + * + * LARGESEND note: !tcp_urg_mode is overkill, only frames between + * snd_up-64k-mss .. snd_up cannot be large. However, taking into + * account rare use of URG, this is not a big flaw. */ static unsigned int tcp_mss_split_point(struct sock *sk, struct sk_buff *skb, unsigned int mss_now, unsigned int cwnd)@@ -1120,6 +1120,9 @@ static unsigned int tcp_mss_split_point(struct sock *sk, struct sk_buff *skb, struct tcp_sock *tp = tcp_sk(sk); u32 needed, window, cwnd_len; + if (unlikely(tcp_urg_mode(tp))) + return mss_now; + window = tcp_wnd_end(tp) - TCP_SKB_CB(skb)->seq; cwnd_len = mss_now * cwnd;
Acked-by: Ilpo Järvinen <redacted> -- i.