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

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

[Ilpo Järvinen]

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..2dc6741 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 (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]

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.