netdev - [PATCH] [TCP]: Add highest_sack_end_seq check back to lost

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.0710181214140.3767@kivilampi-30.cs.helsinki.fi>
Date:	Thu, 18 Oct 2007 12:40:06 +0300 (EEST)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	TAKANO Ryousei <takano@...-inc.co.jp>
cc:	David Miller <davem@...emloft.net>, y-kodama@...t.go.jp,
	Netdev <netdev@...r.kernel.org>
Subject: [PATCH] [TCP]: Add highest_sack_end_seq check back to lost_retrans
 call

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.

> since it misses opportunities of detecting loss of retransmitted packets.

Do you have an idea how it does that except the problem now being fixed?
The lost_retrans_low was supposed to contain the minimum 
TCP_SKB_CB(skb)->ack_seq of those packets that hav TCPCB_SACKED_RETRANS 
set, so if the condition won't match, there shouldn't be any misses. Do 
you think there's something wrong in that approach?

The point of lost_retrans_low is to limit walking in retrans queue to two 
cases:

- There's at least one skb with SACKED_RETRANS to clear (except perhaps 
some corner cases where that specific skb got DSACKed in between and the 
R-bit was therefore cleared).

 or

- New lost_retrans_low has to calculated (should only occur if snd_una 
advanced past it and there are still some retransmissions, not too sure 
if that can occur, and that won't be very likely case anyway).

> "no limit" does as follows:
>         if (tp->retrans_out && highest_sack_end_seq &&
>             icsk->icsk_ca_state == TCP_CA_Recovery)
>                 flag |= tcp_mark_lost_retrans(sk, highest_sack_end_seq);

Having that highest_sack_end_seq there probably solved the problem. Yet 
it won't be good in performance wise like that because many unnecessary 
walks would occur during CA_Recovery, which all are prone to induce some 
cache misses.

--

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

This was unintentionally dropped (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). There won't be anything to mark if SACKs didn't
advance.

It causes passing of zero received_upto to tcp_mark_lost_retrans
which confuses after relations within the marker loop causing
incorrect TCPCB_SACKED_RETRANS clearing.

This 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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0f00966..c3c0183 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1489,7 +1489,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
 		}
 	}

-	if (tp->retrans_out &&
+	if (tp->retrans_out && highest_sack_end_seq &&
 	    after(highest_sack_end_seq, tp->lost_retrans_low) &&
 	    icsk->icsk_ca_state == TCP_CA_Recovery)
 		flag |= tcp_mark_lost_retrans(sk, highest_sack_end_seq);
-- 
1.5.0.6