netdev - [PATCH] [TCP]: Remove lost_retrans zero special cases

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0710181314130.3767@kivilampi-30.cs.helsinki.fi>
Date:	Thu, 18 Oct 2007 13:17:24 +0300 (EEST)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	TAKANO Ryousei <takano@...-inc.co.jp>,
	David Miller <davem@...emloft.net>
cc:	y-kodama@...t.go.jp, Netdev <netdev@...r.kernel.org>
Subject: [PATCH] [TCP]: Remove lost_retrans zero special cases

On Thu, 18 Oct 2007, Ilpo Järvinen wrote:

> On Thu, 18 Oct 2007, TAKANO Ryousei wrote:
> 
> > From: David Miller <davem@...emloft.net>
> > Subject: Re: [PATCH 7/7] [TCP]: Limit processing lost_retrans loop to work-to-do cases
> > Date: Thu, 11 Oct 2007 17:36:22 -0700 (PDT)
> > 
> > > From: "Ilpo_Järvinen" <ilpo.jarvinen@...sinki.fi>
> > > Date: Thu, 11 Oct 2007 14:41:07 +0300
> > > 
> > > > This addition of lost_retrans_low to tcp_sock might be
> > > > unnecessary, it's not clear how often lost_retrans worker is
> > > > executed when there wasn't work to do.
> > > > 
> > > > Cc: TAKANO Ryousei <takano@...-inc.co.jp>
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
> > > 
> > > Applied.
> > 
> > > +	    after(highest_sack_end_seq, tp->lost_retrans_low) &&
> >
> > This limit degrades the performance of my test case described before,
> 
> Thanks for testing.... Btw, just noticed that lost_retrans_low addition 
> patch incorrectly dropped check for highest_sack_end_seq (probably due to 
> incorrect resolution from my side at some point of development of those 
> two patches as I added that check later on when realized it's necessary), 
> patching that below... Since it causes zero received_upto in 
> tcp_mark_lost_retrans, some RETRANS bits got cleared unintentionally
> because of that.

...snip...

> --
> 
> [PATCH] [TCP]: Add highest_sack_end_seq check back to lost_retrans call

Try this patch instead not on the top of the one I sent earlier (IMHO 
this is better approach):

--
[PATCH] [TCP]: Remove lost_retrans zero seqno special cases

Both high-sack detection and new lowest seq variables have
unnecessary zero special case which are now removed by setting
safe initial seqnos.

This also fixes problem which caused zero received_upto being
passed to tcp_mark_lost_retrans which confused after relations
within the marker loop causing incorrect TCPCB_SACKED_RETRANS
clearing. The problem was noticed because of a performance
report from TAKANO Ryousei <takano@...-inc.co.jp>.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
---
 net/ipv4/tcp_input.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0f00966..9288220 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1121,7 +1121,7 @@ static int tcp_mark_lost_retrans(struct sock *sk, u32 received_upto)
 	struct sk_buff *skb;
 	int flag = 0;
 	int cnt = 0;
-	u32 new_low_seq = 0;
+	u32 new_low_seq = tp->snd_nxt;
 
 	tcp_for_write_queue(skb, sk) {
 		u32 ack_seq = TCP_SKB_CB(skb)->ack_seq;
@@ -1153,7 +1153,7 @@ static int tcp_mark_lost_retrans(struct sock *sk, u32 received_upto)
 				NET_INC_STATS_BH(LINUX_MIB_TCPLOSTRETRANSMIT);
 			}
 		} else {
-			if (!new_low_seq || before(ack_seq, new_low_seq))
+			if (before(ack_seq, new_low_seq))
 				new_low_seq = ack_seq;
 			cnt += tcp_skb_pcount(skb);
 		}
@@ -1242,7 +1242,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 	int num_sacks = (ptr[1] - TCPOLEN_SACK_BASE)>>3;
 	int reord = tp->packets_out;
 	int prior_fackets;
-	u32 highest_sack_end_seq = 0;
+	u32 highest_sack_end_seq = tp->lost_retrans_low;
 	int flag = 0;
 	int found_dup_sack = 0;
 	int cached_fack_count;
-- 
1.5.0.6