diff mbox

[net-next] tcp: increase size at which tcp_bound_to_half_wnd bounds to > TCP_MSS_DEFAULT

Message ID CS1PR84MB0295E420892A535E40FE338EFD220@CS1PR84MB0295.NAMPRD84.PROD.OUTLOOK.COM
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Seymour, Shane M June 28, 2016, 4:33 a.m. UTC 
In previous commit 01f83d69844d307be2aa6fea88b0e8fe5cbdb2f4
the following comments were added:

"When peer uses tiny windows, there is no use in packetizing to sub-MSS
pieces for the sake of SWS or making sure there are enough packets in
the pipe for fast recovery."

The test should be > TCP_MSS_DEFAULT not >= 512. This allows low end
devices that send an MSS of 536 (TCP_MSS_DEFAULT) to see better network
performance by sending it 536 bytes of data at a time instead of bounding
to half window size (268). Other network stacks work this way, e.g. HP-UX.

Signed-off-by: Shane Seymour <shane.seymour@hpe.com>
---

Comments

Eric Dumazet June 28, 2016, 11:58 a.m. UTC | #1 
On Tue, 2016-06-28 at 04:33 +0000, Seymour, Shane M wrote:
> In previous commit 01f83d69844d307be2aa6fea88b0e8fe5cbdb2f4
> the following comments were added:
> 
> "When peer uses tiny windows, there is no use in packetizing to sub-MSS
> pieces for the sake of SWS or making sure there are enough packets in
> the pipe for fast recovery."
> 
> The test should be > TCP_MSS_DEFAULT not >= 512. This allows low end
> devices that send an MSS of 536 (TCP_MSS_DEFAULT) to see better network
> performance by sending it 536 bytes of data at a time instead of bounding
> to half window size (268). Other network stacks work this way, e.g. HP-UX.


Trying to cope with ridiculous windows these days is really a waste of
time, as we perform this check for all tcp sendmsg() calls :(

Anyway, your patch is reversed.
Seymour, Shane M June 28, 2016, 9:54 p.m. UTC | #2 
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]

> Trying to cope with ridiculous windows these days is really a waste of

> time, as we perform this check for all tcp sendmsg() calls :(


I don't disagree with you but unfortunately there are still devices
out there like this and probably will be for a long time.

> Anyway, your patch is reversed.


I'm not sure what you mean by reversed, I didn't change the direction
of the test in the code just what it's being compared against and so
it's greater than not greater than or equal to.

Unless auto-correction changed reviewed to reversed?
Neal Cardwell June 28, 2016, 10:35 p.m. UTC | #3 
On Tue, Jun 28, 2016 at 5:54 PM, Seymour, Shane M <shane.seymour@hpe.com> wrote:
>> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
...
>> Anyway, your patch is reversed.
>
> I'm not sure what you mean by reversed, I didn't change the direction
> of the test in the code just what it's being compared against and so
> it's greater than not greater than or equal to.

By reversed, I believe Eric means the following:

Your patch claims that the current line you want to replace says:

-       if (tp->max_window > TCP_MSS_DEFAULT)

And that you want to replace it with this line:

+       if (tp->max_window >= 512)

However, the currrent line is the latter:

   if (tp->max_window >= 512)

And presumably you want to replace it with the former.

It might be best to generate the patch with git.

neal
diff mbox

Patch

--- b/include/net/tcp.h	2016-06-23 20:59:14.521686048 -0500
+++ a/include/net/tcp.h	2016-06-15 17:19:21.964821477 -0500
@@ -589,7 +589,7 @@  static inline int tcp_bound_to_half_wnd(
 	 * On the other hand, for extremely large MSS devices, handling
 	 * smaller than MSS windows in this way does make sense.
 	 */
-	if (tp->max_window > TCP_MSS_DEFAULT)
+	if (tp->max_window >= 512)
 		cutoff = (tp->max_window >> 1);
 	else
 		cutoff = tp->max_window;