Re: [PATCH v3 net-next 2/4] tcp: take care of compressed acks in tcp_add_reno_sack()

2018-11-28 Thread Neal Cardwell
On Tue, Nov 27, 2018 at 5:42 PM Eric Dumazet  wrote:
>
> Neal pointed out that non sack flows might suffer from ACK compression
> added in the following patch ("tcp: implement coalescing on backlog queue")
>
> Instead of tweaking tcp_add_backlog() we can take into
> account how many ACK were coalesced, this information
> will be available in skb_shinfo(skb)->gso_segs
>
> Signed-off-by: Eric Dumazet 
> ---

Acked-by: Neal Cardwell 

Thanks!

neal


[PATCH v3 net-next 2/4] tcp: take care of compressed acks in tcp_add_reno_sack()

2018-11-27 Thread Eric Dumazet
Neal pointed out that non sack flows might suffer from ACK compression
added in the following patch ("tcp: implement coalescing on backlog queue")

Instead of tweaking tcp_add_backlog() we can take into
account how many ACK were coalesced, this information
will be available in skb_shinfo(skb)->gso_segs

Signed-off-by: Eric Dumazet 
---
 net/ipv4/tcp_input.c | 58 +---
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 
f32397890b6dcbc34976954c4be142108efa04d8..e5f3819ad859f6e6ca28e09c0f2dbdb7052708ee
 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1863,16 +1863,20 @@ static void tcp_check_reno_reordering(struct sock *sk, 
const int addend)
 
 /* Emulate SACKs for SACKless connection: account for a new dupack. */
 
-static void tcp_add_reno_sack(struct sock *sk)
+static void tcp_add_reno_sack(struct sock *sk, int num_dupack)
 {
-   struct tcp_sock *tp = tcp_sk(sk);
-   u32 prior_sacked = tp->sacked_out;
+   if (num_dupack) {
+   struct tcp_sock *tp = tcp_sk(sk);
+   u32 prior_sacked = tp->sacked_out;
+   s32 delivered;
 
-   tp->sacked_out++;
-   tcp_check_reno_reordering(sk, 0);
-   if (tp->sacked_out > prior_sacked)
-   tp->delivered++; /* Some out-of-order packet is delivered */
-   tcp_verify_left_out(tp);
+   tp->sacked_out += num_dupack;
+   tcp_check_reno_reordering(sk, 0);
+   delivered = tp->sacked_out - prior_sacked;
+   if (delivered > 0)
+   tp->delivered += delivered;
+   tcp_verify_left_out(tp);
+   }
 }
 
 /* Account for ACK, ACKing some data in Reno Recovery phase. */
@@ -2634,7 +2638,7 @@ void tcp_enter_recovery(struct sock *sk, bool ece_ack)
 /* Process an ACK in CA_Loss state. Move to CA_Open if lost data are
  * recovered or spurious. Otherwise retransmits more on partial ACKs.
  */
-static void tcp_process_loss(struct sock *sk, int flag, bool is_dupack,
+static void tcp_process_loss(struct sock *sk, int flag, int num_dupack,
 int *rexmit)
 {
struct tcp_sock *tp = tcp_sk(sk);
@@ -2653,7 +2657,7 @@ static void tcp_process_loss(struct sock *sk, int flag, 
bool is_dupack,
return;
 
if (after(tp->snd_nxt, tp->high_seq)) {
-   if (flag & FLAG_DATA_SACKED || is_dupack)
+   if (flag & FLAG_DATA_SACKED || num_dupack)
tp->frto = 0; /* Step 3.a. loss was real */
} else if (flag & FLAG_SND_UNA_ADVANCED && !recovered) {
tp->high_seq = tp->snd_nxt;
@@ -2679,8 +2683,8 @@ static void tcp_process_loss(struct sock *sk, int flag, 
bool is_dupack,
/* A Reno DUPACK means new data in F-RTO step 2.b above are
 * delivered. Lower inflight to clock out (re)tranmissions.
 */
-   if (after(tp->snd_nxt, tp->high_seq) && is_dupack)
-   tcp_add_reno_sack(sk);
+   if (after(tp->snd_nxt, tp->high_seq) && num_dupack)
+   tcp_add_reno_sack(sk, num_dupack);
else if (flag & FLAG_SND_UNA_ADVANCED)
tcp_reset_reno_sack(tp);
}
@@ -2757,13 +2761,13 @@ static bool tcp_force_fast_retransmit(struct sock *sk)
  * tcp_xmit_retransmit_queue().
  */
 static void tcp_fastretrans_alert(struct sock *sk, const u32 prior_snd_una,
- bool is_dupack, int *ack_flag, int *rexmit)
+ int num_dupack, int *ack_flag, int *rexmit)
 {
struct inet_connection_sock *icsk = inet_csk(sk);
struct tcp_sock *tp = tcp_sk(sk);
int fast_rexmit = 0, flag = *ack_flag;
-   bool do_lost = is_dupack || ((flag & FLAG_DATA_SACKED) &&
-tcp_force_fast_retransmit(sk));
+   bool do_lost = num_dupack || ((flag & FLAG_DATA_SACKED) &&
+ tcp_force_fast_retransmit(sk));
 
if (!tp->packets_out && tp->sacked_out)
tp->sacked_out = 0;
@@ -2810,8 +2814,8 @@ static void tcp_fastretrans_alert(struct sock *sk, const 
u32 prior_snd_una,
switch (icsk->icsk_ca_state) {
case TCP_CA_Recovery:
if (!(flag & FLAG_SND_UNA_ADVANCED)) {
-   if (tcp_is_reno(tp) && is_dupack)
-   tcp_add_reno_sack(sk);
+   if (tcp_is_reno(tp))
+   tcp_add_reno_sack(sk, num_dupack);
} else {
if (tcp_try_undo_partial(sk, prior_snd_una))
return;
@@ -2826,7 +2830,7 @@ static void tcp_fastretrans_alert(struct sock *sk, const 
u32 prior_snd_una,
tcp_identify_packet_loss(sk, ack_flag);
br