[PATCH] apply cwnd rules to FIN packets with data

[PATCH] apply cwnd rules to FIN packets with data

[John Heffner]

[-- Attachment #1: Type: text/plain, Size: 281 bytes --]

This is especially important with TSO enabled.  Currently, it will send 
a burst of up to 64k at the end of a connection, even when cwnd is much 
smaller than 64k.  This patch still lets out empty FIN packets, but does 
not apply the special case to FINs carrying data.

   -John


[-- Attachment #2: fin_cwnd.patch --]
[-- Type: text/plain, Size: 1545 bytes --]

Apply cwnd rules to FIN packets that contain data.

---
commit af319609eee705e0791a1a58c33b216e8d0254bf
tree 5a1afcc506e09f5adfd74efb7e0cbbc82ec4d5b0
parent c0d4d573feed199b16094c072e7cb07afb01c598
author John Heffner <jheffner@psc.edu> Mon, 05 Feb 2007 16:25:46 -0500
committer John Heffner <jheffner@psc.edu> Mon, 05 Feb 2007 16:25:46 -0500

 net/ipv4/tcp_output.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 975f447..215c99d 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -965,7 +965,7 @@ static inline unsigned int tcp_cwnd_test
 	u32 in_flight, cwnd;
 
 	/* Don't be strict about the congestion window for the final FIN.  */
-	if (TCP_SKB_CB(skb)->flags & TCPCB_FLAG_FIN)
+	if ((TCP_SKB_CB(skb)->flags & TCPCB_FLAG_FIN) && skb->len == 0)
 		return 1;
 
 	in_flight = tcp_packets_in_flight(tp);
@@ -1034,7 +1034,7 @@ static inline int tcp_nagle_test(struct 
 
 	/* Don't use the nagle rule for urgent data (or for the final FIN).  */
 	if (tp->urg_mode ||
-	    (TCP_SKB_CB(skb)->flags & TCPCB_FLAG_FIN))
+	    ((TCP_SKB_CB(skb)->flags & TCPCB_FLAG_FIN) && skb->len == 0))
 		return 1;
 
 	if (!tcp_nagle_check(tp, skb, cur_mss, nonagle))
@@ -1156,9 +1156,6 @@ static int tcp_tso_should_defer(struct s
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 	u32 send_win, cong_win, limit, in_flight;
 
-	if (TCP_SKB_CB(skb)->flags & TCPCB_FLAG_FIN)
-		goto send_now;
-
 	if (icsk->icsk_ca_state != TCP_CA_Open)
 		goto send_now;
 

Re: [PATCH] apply cwnd rules to FIN packets with data

[David Miller]

From: John Heffner <jheffner@psc.edu>
Date: Mon, 05 Feb 2007 16:58:18 -0500

> This is especially important with TSO enabled.  Currently, it will send 
> a burst of up to 64k at the end of a connection, even when cwnd is much 
> smaller than 64k.  This patch still lets out empty FIN packets, but does 
> not apply the special case to FINs carrying data.

Good catch John.

But I think the correct test on skb->len would be to just make
sure that it is <= REAL_MSS.

What do you think about that?  This would match the original intention
of the logic in the pre-TSO days.

Re: [PATCH] apply cwnd rules to FIN packets with data

[John Heffner]

David Miller wrote:
> From: John Heffner <jheffner@psc.edu>
> Date: Mon, 05 Feb 2007 16:58:18 -0500
> 
>> This is especially important with TSO enabled.  Currently, it will send 
>> a burst of up to 64k at the end of a connection, even when cwnd is much 
>> smaller than 64k.  This patch still lets out empty FIN packets, but does 
>> not apply the special case to FINs carrying data.
> 
> Good catch John.
> 
> But I think the correct test on skb->len would be to just make
> sure that it is <= REAL_MSS.
> 
> What do you think about that?  This would match the original intention
> of the logic in the pre-TSO days.

What was the intention of that logic?

Actually, I think it would be better to leave the Nagle test as it was 
(which is implicitly < real_mss), because there is obviously no point in 
doing the nagle test when you know there is no more data that will be 
sent.  However, I can't think of any reason why the cwnd test should not 
apply.

   -John

Re: [PATCH] apply cwnd rules to FIN packets with data

[David Miller]

From: John Heffner <jheffner@psc.edu>
Date: Mon, 05 Feb 2007 18:02:19 -0500

> David Miller wrote:
> > From: John Heffner <jheffner@psc.edu>
> > Date: Mon, 05 Feb 2007 16:58:18 -0500
> > 
> >> This is especially important with TSO enabled.  Currently, it will send 
> >> a burst of up to 64k at the end of a connection, even when cwnd is much 
> >> smaller than 64k.  This patch still lets out empty FIN packets, but does 
> >> not apply the special case to FINs carrying data.
> > 
> > Good catch John.
> > 
> > But I think the correct test on skb->len would be to just make
> > sure that it is <= REAL_MSS.
> > 
> > What do you think about that?  This would match the original intention
> > of the logic in the pre-TSO days.
> 
> What was the intention of that logic?

Because a packet is a packet is a packet.

If we let a FIN out it's basically equivalent to a FIN+data
as far as the routers are concerned.  Either they will drop
the packet or they won't.  And they will do this regardless
of whether data is attached to the FIN.

Getting FINs out fast is important for another reason, the
faster the session closes the faster "smart" intermediate
routers can teardown cached routing or firewall tracking
entries for the flow.

> Actually, I think it would be better to leave the Nagle test as it was 
> (which is implicitly < real_mss), because there is obviously no point in 
> doing the nagle test when you know there is no more data that will be 
> sent.

True.

> However, I can't think of any reason why the cwnd test should not 
> apply.

Care to elaborate here?  You can view the FIN special case as an off
by one error in the CWND test, it's not going to melt the internet.
:-)

Re: [PATCH] apply cwnd rules to FIN packets with data

[John Heffner]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

David Miller wrote:
>> However, I can't think of any reason why the cwnd test should not 
>> apply.
> 
> Care to elaborate here?  You can view the FIN special case as an off
> by one error in the CWND test, it's not going to melt the internet.
> :-)

True, it's not going to melt the internet, but why stop at one when two 
would finish the connection even faster?  Not sure I buy this argument. 
  Was there some benchmarking data that was a justification for this in 
the first place?

My first patch was broken anyway (should not have pulled the test from 
tso_should_defer), and the change is not needed to the nagle test since 
it's implicit.  This patch just restores the old behavior from before 
TSO, sending the FIN when it's the last true segment.  We can debate the 
merits of applying congestion control to the FIN separately. :)

   -John

[-- Attachment #2: fin_cwnd1.patch --]
[-- Type: text/plain, Size: 963 bytes --]

Don't apply FIN exception to full TSO segments.

Signed-off-by: John Heffner <jheffner@psc.edu>

---
commit 89de0d8cb75958b0315c076b31a597143e30f7a4
tree 7e9c321e62729c6ef76e3886fe9edf2ac78a680c
parent c0d4d573feed199b16094c072e7cb07afb01c598
author John Heffner <jheffner@psc.edu> Mon, 05 Feb 2007 18:42:31 -0500
committer John Heffner <jheffner@psc.edu> Mon, 05 Feb 2007 18:42:31 -0500

 net/ipv4/tcp_output.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 975f447..58b7111 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -965,7 +965,8 @@ static inline unsigned int tcp_cwnd_test
 	u32 in_flight, cwnd;
 
 	/* Don't be strict about the congestion window for the final FIN.  */
-	if (TCP_SKB_CB(skb)->flags & TCPCB_FLAG_FIN)
+	if ((TCP_SKB_CB(skb)->flags & TCPCB_FLAG_FIN) &&
+	    tcp_skb_pcount(skb) == 1)
 		return 1;
 
 	in_flight = tcp_packets_in_flight(tp);

Re: [PATCH] apply cwnd rules to FIN packets with data

[Rick Jones]

John Heffner wrote:
> David Miller wrote:
> 
>>> However, I can't think of any reason why the cwnd test should not apply.
>>
>>
>> Care to elaborate here?  You can view the FIN special case as an off
>> by one error in the CWND test, it's not going to melt the internet.
>> :-)
> 
> 
> True, it's not going to melt the internet, but why stop at one when two 
> would finish the connection even faster?  Not sure I buy this argument. 
>  Was there some benchmarking data that was a justification for this in 
> the first place?

Is the cwnd in the stack byte based, or packet based?

While "all" the RFCs tend to discuss things in terms of byte-based cwnds 
and assumptions based on MSSes and such, the underlying principle was/is 
a conservation of packets.  As David said, a packet is a packet, and if 
one were going to be sending a FIN segment, it might as well carry data. 
  And if one isn't comfortable sending that one last data segment with 
the FIN because cwnd wasn't large enough at the time, should the FIN be 
sent at that point, even if it is waffer thin?

rick jones
2 cents tossed-in from the peanut gallery

Re: [PATCH] apply cwnd rules to FIN packets with data

[John Heffner]

Rick Jones wrote:
> John Heffner wrote:
>> David Miller wrote:
>>
>>>> However, I can't think of any reason why the cwnd test should not 
>>>> apply.
>>>
>>>
>>> Care to elaborate here?  You can view the FIN special case as an off
>>> by one error in the CWND test, it's not going to melt the internet.
>>> :-)
>>
>>
>> True, it's not going to melt the internet, but why stop at one when 
>> two would finish the connection even faster?  Not sure I buy this 
>> argument.  Was there some benchmarking data that was a justification 
>> for this in the first place?
> 
> Is the cwnd in the stack byte based, or packet based?
> 
> While "all" the RFCs tend to discuss things in terms of byte-based cwnds 
> and assumptions based on MSSes and such, the underlying principle was/is 
> a conservation of packets.  As David said, a packet is a packet, and if 
> one were going to be sending a FIN segment, it might as well carry data. 
>  And if one isn't comfortable sending that one last data segment with 
> the FIN because cwnd wasn't large enough at the time, should the FIN be 
> sent at that point, even if it is waffer thin?

The most conservative thing is to apply congestion control exactly as 
you would to any other segment, that is, just take the special case out 
entirely.  An empty FIN is not too likely to cause problems, a full-MSS 
FIN somewhat more so, 2-MSS, yet more, a 64k TSO segment even more. :) 
I don't have hard data to argue for or against any particular 
optimization, but it seems there should be some if we're ignoring the 
standard cwnd rules.

   -John

Re: [PATCH] apply cwnd rules to FIN packets with data

[David Miller]

From: John Heffner <jheffner@psc.edu>
Date: Mon, 05 Feb 2007 19:11:09 -0500

> My first patch was broken anyway (should not have pulled the test from 
> tso_should_defer), and the change is not needed to the nagle test since 
> it's implicit.  This patch just restores the old behavior from before 
> TSO, sending the FIN when it's the last true segment.  We can debate the 
> merits of applying congestion control to the FIN separately. :)

Yes, let's split one hair at a time :-)

Patch applied, thanks John.  I'll push this to the 2.6.x -stable
branch too.