[PATCH 7/7] tcp: make sure xmit goal size never becomes zero

[PATCH 7/7] tcp: make sure xmit goal size never becomes zero

[Ilpo Järvinen]

It's not too likely to happen, would basically require crafted
packets (must hit the max guard in tcp_bound_to_half_wnd()).
It seems that nothing that bad would happen as there's tcp_mems
and congestion window that prevent runaway at some point from
hurting all too much (I'm not that sure what all those zero
sized segments we would generate do though in write queue).
Preventing it regardless is certainly the best way to go.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Ingo Molnar <mingo@elte.hu>
---
 net/ipv4/tcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 2dc6741..274ab99 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -689,7 +689,7 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
 		}
 	}
 
-	return xmit_size_goal;
+	return max(xmit_size_goal, mss_now);
 }
 
 static int tcp_send_mss(struct sock *sk, int *size_goal, int flags)
-- 
1.5.6.5

Re: [PATCH 6/7] tcp: cache result of earlier divides when mss-aligning things

[Evgeniy Polyakov]

Hi Ilpo.

On Sun, Mar 15, 2009 at 02:07:54AM +0200, Ilpo Järvinen (ilpo.jarvinen@helsinki.fi) wrote:
> @@ -676,7 +676,17 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
>  				  tp->tcp_header_len);
>  
>  		xmit_size_goal = tcp_bound_to_half_wnd(tp, xmit_size_goal);
> -		xmit_size_goal -= (xmit_size_goal % mss_now);
> +
> +		/* We try hard to avoid divides here */
> +		old_size_goal = tp->xmit_size_goal_segs * mss_now;
> +
> +		if (old_size_goal <= xmit_size_goal &&
> +		    old_size_goal + mss_now > xmit_size_goal) {
> +			xmit_size_goal = old_size_goal;

If this is way more likely condition than changed xmit size, what about
wrapping it into likely()?

> +		} else {
> +			tp->xmit_size_goal_segs = xmit_size_goal / mss_now;
> +			xmit_size_goal = tp->xmit_size_goal_segs * mss_now;
> +		}
>  	}
>  
>  	return xmit_size_goal;
> -- 
> 1.5.6.5

-- 
	Evgeniy Polyakov

Re: [PATCH 6/7] tcp: cache result of earlier divides when mss-aligning things

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2998 bytes --]

On Sun, 15 Mar 2009, Evgeniy Polyakov wrote:

> On Sun, Mar 15, 2009 at 02:07:54AM +0200, Ilpo Järvinen (ilpo.jarvinen@helsinki.fi) wrote:
> > @@ -676,7 +676,17 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
> >  				  tp->tcp_header_len);
> >  
> >  		xmit_size_goal = tcp_bound_to_half_wnd(tp, xmit_size_goal);
> > -		xmit_size_goal -= (xmit_size_goal % mss_now);
> > +
> > +		/* We try hard to avoid divides here */
> > +		old_size_goal = tp->xmit_size_goal_segs * mss_now;
> > +
> > +		if (old_size_goal <= xmit_size_goal &&
> > +		    old_size_goal + mss_now > xmit_size_goal) {
> > +			xmit_size_goal = old_size_goal;
> 
> If this is way more likely condition than changed xmit size, what about
> wrapping it into likely()?

So gcc won't read my comment? :-)

Updated below.

-- 
 i.

--
[PATCHv2] tcp: cache result of earlier divides when mss-aligning things

The results is very unlikely change every so often so we
hardly need to divide again after doing that once for a
connection. Yet, if divide still becomes necessary we
detect that and do the right thing and again settle for
non-divide state. Takes the u16 space which was previously
taken by the plain xmit_size_goal.

This should take care part of the tso vs non-tso difference
we found earlier.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: Ingo Molnar <mingo@elte.hu>
---
 include/linux/tcp.h |    1 +
 net/ipv4/tcp.c      |   14 ++++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index ad2021c..9d5078b 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -248,6 +248,7 @@ struct tcp_sock {
 	/* inet_connection_sock has to be the first member of tcp_sock */
 	struct inet_connection_sock	inet_conn;
 	u16	tcp_header_len;	/* Bytes of tcp header to send		*/
+	u16	xmit_size_goal_segs; /* Goal for segmenting output packets */
 
 /*
  *	Header prediction flags
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 886596f..0db9f3b 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -665,7 +665,7 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
 				       int large_allowed)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	u32 xmit_size_goal;
+	u32 xmit_size_goal, old_size_goal;
 
 	xmit_size_goal = mss_now;
 
@@ -676,7 +676,17 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
 				  tp->tcp_header_len);
 
 		xmit_size_goal = tcp_bound_to_half_wnd(tp, xmit_size_goal);
-		xmit_size_goal -= (xmit_size_goal % mss_now);
+
+		/* We try hard to avoid divides here */
+		old_size_goal = tp->xmit_size_goal_segs * mss_now;
+
+		if (likely(old_size_goal <= xmit_size_goal &&
+			   old_size_goal + mss_now > xmit_size_goal)) {
+			xmit_size_goal = old_size_goal;
+		} else {
+			tp->xmit_size_goal_segs = xmit_size_goal / mss_now;
+			xmit_size_goal = tp->xmit_size_goal_segs * mss_now;
+		}
 	}
 
 	return xmit_size_goal;
-- 
1.5.6.5

Re: [PATCH 6/7] tcp: cache result of earlier divides when mss-aligning things

[Evgeniy Polyakov]

On Sun, Mar 15, 2009 at 10:45:16AM +0200, Ilpo Järvinen (ilpo.jarvinen@helsinki.fi) wrote:
> > > @@ -676,7 +676,17 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
> > >  				  tp->tcp_header_len);
> > >  
> > >  		xmit_size_goal = tcp_bound_to_half_wnd(tp, xmit_size_goal);
> > > -		xmit_size_goal -= (xmit_size_goal % mss_now);
> > > +
> > > +		/* We try hard to avoid divides here */
> > > +		old_size_goal = tp->xmit_size_goal_segs * mss_now;
> > > +
> > > +		if (old_size_goal <= xmit_size_goal &&
> > > +		    old_size_goal + mss_now > xmit_size_goal) {
> > > +			xmit_size_goal = old_size_goal;
> > 
> > If this is way more likely condition than changed xmit size, what about
> > wrapping it into likely()?
> 
> So gcc won't read my comment? :-)

I heared the next gcc version will be linked with the libastral.so, but
we have to maintain backward compatibility.

> Updated below.

The whole series looks good.

-- 
	Evgeniy Polyakov

Re: [PATCH 1/7] tcp: remove pointless .dsack/.num_sacks code

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 15 Mar 2009 02:07:49 +0200

> In the pure assignment case, the earlier zeroing is
> still in effect.
> 
> David S. Miller raised concerns if the ifs are there to avoid
> dirtying cachelines. I came to these conclusions:
> 
> > We'll be dirty it anyway (now that I check), the first "real" statement
> > in tcp_rcv_established is:
> >
> >       tp->rx_opt.saw_tstamp = 0;
> >
> > ...that'll land on the same dword. :-/
> >
> > I suppose the blocks are there just because they had more complexity
> > inside when they had to calculate the eff_sacks too (maybe it would
> > have been better to just remove them in that drop-patch so you would
> > have had less head-ache :-)).
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 2/7] tcp: kill dead end_seq variable in clean_rtx_queue

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 15 Mar 2009 02:07:50 +0200

> I've already forgotten what for this was necessary, anyway
> it's no longer used (if it ever was).
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 3/7] tcp: consolidate paws check

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 15 Mar 2009 02:07:51 +0200

> Wow, it was quite tricky to merge that stream of negations
> but I think I finally got it right:
> 
> check & replace_ts_recent:
> (s32)(rcv_tsval - ts_recent) >= 0                  => 0
> (s32)(ts_recent - rcv_tsval) <= 0                  => 0
> 
> discard:
> (s32)(ts_recent - rcv_tsval)  > TCP_PAWS_WINDOW    => 1
> (s32)(ts_recent - rcv_tsval) <= TCP_PAWS_WINDOW    => 0
> 
> I toggled the return values of tcp_paws_check around since
> the old encoding added yet-another negation making tracking
> of truth-values really complicated.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 4/7] tcp: don't check mtu probe completion in the loop

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 15 Mar 2009 02:07:52 +0200

> It seems that no variables clash such that we couldn't do
> the check just once later on. Therefore move it.
> 
> Also kill dead obvious comment, dead argument and add
> unlikely since this mtu probe does not happen too often.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 5/7] tcp: simplify tcp_current_mss

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 15 Mar 2009 02:07:53 +0200

> There's very little need for most of the callsites to get
> tp->xmit_goal_size updated. That will cost us divide as is,
> so slice the function in two. Also, the only users of the
> tp->xmit_goal_size are directly behind tcp_current_mss(),
> so there's no need to store that variable into tcp_sock
> at all! The drop of xmit_goal_size currently leaves 16-bit
> hole and some reorganization would again be necessary to
> change that (but I'm aiming to fill that hole with u16
> xmit_goal_size_segs to cache the results of the remaining
> divide to get that tso on regression).
> 
> Bring xmit_goal_size parts into tcp.c
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 6/7] tcp: cache result of earlier divides when mss-aligning things

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 15 Mar 2009 10:45:16 +0200 (EET)

> [PATCHv2] tcp: cache result of earlier divides when mss-aligning things
> 
> The results is very unlikely change every so often so we
> hardly need to divide again after doing that once for a
> connection. Yet, if divide still becomes necessary we
> detect that and do the right thing and again settle for
> non-divide state. Takes the u16 space which was previously
> taken by the plain xmit_size_goal.
> 
> This should take care part of the tso vs non-tso difference
> we found earlier.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.

Re: [PATCH 7/7] tcp: make sure xmit goal size never becomes zero

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Sun, 15 Mar 2009 02:07:55 +0200

> It's not too likely to happen, would basically require crafted
> packets (must hit the max guard in tcp_bound_to_half_wnd()).
> It seems that nothing that bad would happen as there's tcp_mems
> and congestion window that prevent runaway at some point from
> hurting all too much (I'm not that sure what all those zero
> sized segments we would generate do though in write queue).
> Preventing it regardless is certainly the best way to go.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Applied.