Re: Refactor TCP partial ACK handling
On Tue, Oct 24, 2017 at 03:21:08PM +0200, Mike Belopuhov wrote: > I didn't do it because tcp_var.h is where tcp keeps all of it's prototypes > but I don't mind moving them into tcp_input.c. Any objections? Otherwise > I'll check in the diff below. ok job@
Re: Refactor TCP partial ACK handling
On Tue, Oct 24, 2017 at 03:21:08PM +0200, Mike Belopuhov wrote:
> I didn't do it because tcp_var.h is where tcp keeps all of it's prototypes
> but I don't mind moving them into tcp_input.c. Any objections? Otherwise
> I'll check in the diff below.
Regression tests pass.
OK bluhm@
> diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
> index 790e163975e..8d172e2905c 100644
> --- sys/netinet/tcp_input.c
> +++ sys/netinet/tcp_input.c
> @@ -183,10 +183,13 @@ do { \
> else \
> TCP_SET_DELACK(tp); \
> if_put(ifp); \
> } while (0)
>
> +void tcp_sack_partialack(struct tcpcb *, struct tcphdr *);
> +void tcp_newreno_partialack(struct tcpcb *, struct tcphdr *);
> +
> void syn_cache_put(struct syn_cache *);
> void syn_cache_rm(struct syn_cache *);
> int syn_cache_respond(struct syn_cache *, struct mbuf *);
> void syn_cache_timer(void *);
> void syn_cache_reaper(void *);
> @@ -1664,52 +1667,39 @@ trimthenstep6:
> }
> /*
>* If the congestion window was inflated to account
>* for the other side's cached packets, retract it.
>*/
> - if (tp->sack_enable) {
> - if (tp->t_dupacks >= tcprexmtthresh) {
> - /* Check for a partial ACK */
> - if (tcp_sack_partialack(tp, th)) {
> -#ifdef TCP_FACK
> - /* Force call to tcp_output */
> - if (tp->snd_awnd < tp->snd_cwnd)
> - tp->t_flags |= TF_NEEDOUTPUT;
> -#else
> - tp->snd_cwnd += tp->t_maxseg;
> - tp->t_flags |= TF_NEEDOUTPUT;
> -#endif /* TCP_FACK */
> - } else {
> - /* Out of fast recovery */
> - tp->snd_cwnd = tp->snd_ssthresh;
> - if (tcp_seq_subtract(tp->snd_max,
> - th->th_ack) < tp->snd_ssthresh)
> - tp->snd_cwnd =
> -tcp_seq_subtract(tp->snd_max,
> -th->th_ack);
> - tp->t_dupacks = 0;
> -#ifdef TCP_FACK
> - if (SEQ_GT(th->th_ack, tp->snd_fack))
> - tp->snd_fack = th->th_ack;
> -#endif
> - }
> - }
> - } else {
> - if (tp->t_dupacks >= tcprexmtthresh &&
> - !tcp_newreno(tp, th)) {
> + if (tp->t_dupacks >= tcprexmtthresh) {
> + /* Check for a partial ACK */
> + if (SEQ_LT(th->th_ack, tp->snd_last)) {
> + if (tp->sack_enable)
> + tcp_sack_partialack(tp, th);
> + else
> + tcp_newreno_partialack(tp, th);
> + } else {
> /* Out of fast recovery */
> tp->snd_cwnd = tp->snd_ssthresh;
> if (tcp_seq_subtract(tp->snd_max, th->th_ack) <
> tp->snd_ssthresh)
> tp->snd_cwnd =
> tcp_seq_subtract(tp->snd_max,
> th->th_ack);
> tp->t_dupacks = 0;
> +#ifdef TCP_FACK
> + if (tp->sack_enable &&
> + SEQ_GT(th->th_ack, tp->snd_fack))
> + tp->snd_fack = th->th_ack;
> +#endif
> }
> - }
> - if (tp->t_dupacks < tcprexmtthresh)
> + } else {
> + /*
> + * Reset the duplicate ACK counter if we
> + * were not in fast recovery.
> + */
> tp->t_dupacks = 0;
> + }
> if (SEQ_GT(th->th_ack, tp->snd_max)) {
> tcpstat_inc(tcps_rcvacktoomuch);
> goto dropafterack_ratelim;
> }
> acked = th->th_ack - tp->snd_una;
> @@ -2703,36 +2693,38 @@ tcp_clean_sackreport(struct tcpcb *tp)
> tp->sackblks[i].start = tp->sackblks[i].end=0;
>
> }
>
> /*
> - * Checks for partial ack. If partial ack arrives, turn off retransmission
> - * timer, deflate the window, do not clear tp->t_dupacks, and return 1.
> - * If the ack advances at least to tp->snd_last, return 0.
> + * Partial ack handling within a sack recovery episode. When a partial ack
> + * arrives, turn off
Re: Refactor TCP partial ACK handling
On Tue, Oct 24, 2017 at 13:37 +0200, Martin Pieuchot wrote:
> On 24/10/17(Tue) 12:27, Mike Belopuhov wrote:
> > On Tue, Oct 24, 2017 at 12:05 +0200, Martin Pieuchot wrote:
> > > On 21/10/17(Sat) 15:17, Mike Belopuhov wrote:
> > > > On Fri, Oct 20, 2017 at 22:59 +0200, Klemens Nanni wrote:
> > > > > The comments for both void tcp_{sack,newreno}_partialack() still
> > > > > mention
> > > > > tp->snd_last and return value bits.
> > > > >
> > > >
> > > > Good eyes! It made me spot a mistake I made by folding two lines
> > > > into an incorrect ifdef in tcp_sack_partialack. I expected it to
> > > > say "ifdef TCP_FACK" while it says "ifNdef". The adjusted comment
> > > > didn't make sense and I found the bug.
> > >
> > > Could you send the full/fixed diff?
> > >
> >
> > Sure.
>
> Diff is correct. I have two suggestions, but it's ok mpi@ either way.
>
> > > And what about TCP_FACK? It is disabled by default, is there a
> > > point in keeping it?
> >
> > Job has pointed out that RFC 6675 might be a better alternative
> > so it might be a good idea to ditch it while we're at it. I'm
> > not certain which parts need to be preserved (if any) however.
>
> I'd say remove it. One can always look in the Attic if necessary.
>
> > diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
> > index 790e163975e..3809a2371f2 100644
> > --- sys/netinet/tcp_input.c
> > +++ sys/netinet/tcp_input.c
> > @@ -1664,52 +1664,38 @@ trimthenstep6:
> > }
> > /*
> > * If the congestion window was inflated to account
> > * for the other side's cached packets, retract it.
> > */
> > - if (tp->sack_enable) {
> > - if (tp->t_dupacks >= tcprexmtthresh) {
> > - /* Check for a partial ACK */
> > - if (tcp_sack_partialack(tp, th)) {
> > -#ifdef TCP_FACK
> > - /* Force call to tcp_output */
> > - if (tp->snd_awnd < tp->snd_cwnd)
> > - tp->t_flags |= TF_NEEDOUTPUT;
> > -#else
> > - tp->snd_cwnd += tp->t_maxseg;
> > - tp->t_flags |= TF_NEEDOUTPUT;
> > -#endif /* TCP_FACK */
> > - } else {
> > - /* Out of fast recovery */
> > - tp->snd_cwnd = tp->snd_ssthresh;
> > - if (tcp_seq_subtract(tp->snd_max,
> > - th->th_ack) < tp->snd_ssthresh)
> > - tp->snd_cwnd =
> > - tcp_seq_subtract(tp->snd_max,
> > - th->th_ack);
> > - tp->t_dupacks = 0;
> > -#ifdef TCP_FACK
> > - if (SEQ_GT(th->th_ack, tp->snd_fack))
> > - tp->snd_fack = th->th_ack;
> > -#endif
> > - }
> > - }
> > - } else {
> > - if (tp->t_dupacks >= tcprexmtthresh &&
> > - !tcp_newreno(tp, th)) {
> > + if (tp->t_dupacks >= tcprexmtthresh) {
>
> I'd keep the comment:
>
> /* Check for a partial ACK */
Sure.
> > diff --git sys/netinet/tcp_var.h sys/netinet/tcp_var.h
> > index 6b797fd48e7..97b04884879 100644
> > --- sys/netinet/tcp_var.h
> > +++ sys/netinet/tcp_var.h
> > @@ -764,15 +764,15 @@ void tcp_update_sack_list(struct tcpcb *tp,
> > tcp_seq, tcp_seq);
> > voidtcp_del_sackholes(struct tcpcb *, struct tcphdr *);
> > voidtcp_clean_sackreport(struct tcpcb *tp);
> > voidtcp_sack_adjust(struct tcpcb *tp);
> > struct sackhole *
> > tcp_sack_output(struct tcpcb *tp);
> > -int tcp_sack_partialack(struct tcpcb *, struct tcphdr *);
> > +voidtcp_sack_partialack(struct tcpcb *, struct tcphdr *);
> > +voidtcp_newreno_partialack(struct tcpcb *, struct tcphdr *);
> > #ifdef DEBUG
> > voidtcp_print_holes(struct tcpcb *tp);
> > #endif
> > -int tcp_newreno(struct tcpcb *, struct tcphdr *);
>
> I'd love to see these definitions moved to netinet/tcp_input.c. It
> makes code audit much more simpler as you know that their are local.
>
I didn't do it because tcp_var.h is where tcp keeps all of it's prototypes
but I don't mind moving them into tcp_input.c. Any objections? Otherwise
I'll check in the diff below.
diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
index 790e163975e..8d172e2905c 100644
--- sys/netinet/tcp_input.c
+++ sys/netinet/tcp_input.c
@@ -183,10 +183,13 @@ do { \
else \
TCP_SET_DELACK(tp); \
if_put(ifp); \
} while (0)
+voidtcp_sack_partialack(struct tcpcb *, struct tcphdr *);
+void
Re: Refactor TCP partial ACK handling
On 24/10/17(Tue) 12:27, Mike Belopuhov wrote:
> On Tue, Oct 24, 2017 at 12:05 +0200, Martin Pieuchot wrote:
> > On 21/10/17(Sat) 15:17, Mike Belopuhov wrote:
> > > On Fri, Oct 20, 2017 at 22:59 +0200, Klemens Nanni wrote:
> > > > The comments for both void tcp_{sack,newreno}_partialack() still mention
> > > > tp->snd_last and return value bits.
> > > >
> > >
> > > Good eyes! It made me spot a mistake I made by folding two lines
> > > into an incorrect ifdef in tcp_sack_partialack. I expected it to
> > > say "ifdef TCP_FACK" while it says "ifNdef". The adjusted comment
> > > didn't make sense and I found the bug.
> >
> > Could you send the full/fixed diff?
> >
>
> Sure.
Diff is correct. I have two suggestions, but it's ok mpi@ either way.
> > And what about TCP_FACK? It is disabled by default, is there a
> > point in keeping it?
>
> Job has pointed out that RFC 6675 might be a better alternative
> so it might be a good idea to ditch it while we're at it. I'm
> not certain which parts need to be preserved (if any) however.
I'd say remove it. One can always look in the Attic if necessary.
> diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
> index 790e163975e..3809a2371f2 100644
> --- sys/netinet/tcp_input.c
> +++ sys/netinet/tcp_input.c
> @@ -1664,52 +1664,38 @@ trimthenstep6:
> }
> /*
>* If the congestion window was inflated to account
>* for the other side's cached packets, retract it.
>*/
> - if (tp->sack_enable) {
> - if (tp->t_dupacks >= tcprexmtthresh) {
> - /* Check for a partial ACK */
> - if (tcp_sack_partialack(tp, th)) {
> -#ifdef TCP_FACK
> - /* Force call to tcp_output */
> - if (tp->snd_awnd < tp->snd_cwnd)
> - tp->t_flags |= TF_NEEDOUTPUT;
> -#else
> - tp->snd_cwnd += tp->t_maxseg;
> - tp->t_flags |= TF_NEEDOUTPUT;
> -#endif /* TCP_FACK */
> - } else {
> - /* Out of fast recovery */
> - tp->snd_cwnd = tp->snd_ssthresh;
> - if (tcp_seq_subtract(tp->snd_max,
> - th->th_ack) < tp->snd_ssthresh)
> - tp->snd_cwnd =
> -tcp_seq_subtract(tp->snd_max,
> -th->th_ack);
> - tp->t_dupacks = 0;
> -#ifdef TCP_FACK
> - if (SEQ_GT(th->th_ack, tp->snd_fack))
> - tp->snd_fack = th->th_ack;
> -#endif
> - }
> - }
> - } else {
> - if (tp->t_dupacks >= tcprexmtthresh &&
> - !tcp_newreno(tp, th)) {
> + if (tp->t_dupacks >= tcprexmtthresh) {
I'd keep the comment:
/* Check for a partial ACK */
> + if (SEQ_LT(th->th_ack, tp->snd_last)) {
> + if (tp->sack_enable)
> + tcp_sack_partialack(tp, th);
> + else
> + tcp_newreno_partialack(tp, th);
> + } else {
> /* Out of fast recovery */
> tp->snd_cwnd = tp->snd_ssthresh;
> if (tcp_seq_subtract(tp->snd_max, th->th_ack) <
> tp->snd_ssthresh)
> tp->snd_cwnd =
> tcp_seq_subtract(tp->snd_max,
> th->th_ack);
> tp->t_dupacks = 0;
> +#ifdef TCP_FACK
> + if (tp->sack_enable &&
> + SEQ_GT(th->th_ack, tp->snd_fack))
> + tp->snd_fack = th->th_ack;
> +#endif
> }
> - }
> - if (tp->t_dupacks < tcprexmtthresh)
> + } else {
> + /*
> + * Reset the duplicate ACK counter if we
> + * were not in fast recovery.
> + */
> tp->t_dupacks = 0;
> + }
> if (SEQ_GT(th->th_ack, tp->snd_max)) {
> tcpstat_inc(tcps_rcvacktoomuch);
> goto dropafterack_ratelim;
> }
> acked = th->th_ack - tp->snd_una;
> @@ -2703,36 +2689,38 @@ tcp_clean_sackreport(struct tcpcb *tp)
>
Re: Refactor TCP partial ACK handling
On Tue, Oct 24, 2017 at 12:05 +0200, Martin Pieuchot wrote:
> On 21/10/17(Sat) 15:17, Mike Belopuhov wrote:
> > On Fri, Oct 20, 2017 at 22:59 +0200, Klemens Nanni wrote:
> > > The comments for both void tcp_{sack,newreno}_partialack() still mention
> > > tp->snd_last and return value bits.
> > >
> >
> > Good eyes! It made me spot a mistake I made by folding two lines
> > into an incorrect ifdef in tcp_sack_partialack. I expected it to
> > say "ifdef TCP_FACK" while it says "ifNdef". The adjusted comment
> > didn't make sense and I found the bug.
>
> Could you send the full/fixed diff?
>
Sure.
> And what about TCP_FACK? It is disabled by default, is there a
> point in keeping it?
Job has pointed out that RFC 6675 might be a better alternative
so it might be a good idea to ditch it while we're at it. I'm
not certain which parts need to be preserved (if any) however.
diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
index 790e163975e..3809a2371f2 100644
--- sys/netinet/tcp_input.c
+++ sys/netinet/tcp_input.c
@@ -1664,52 +1664,38 @@ trimthenstep6:
}
/*
* If the congestion window was inflated to account
* for the other side's cached packets, retract it.
*/
- if (tp->sack_enable) {
- if (tp->t_dupacks >= tcprexmtthresh) {
- /* Check for a partial ACK */
- if (tcp_sack_partialack(tp, th)) {
-#ifdef TCP_FACK
- /* Force call to tcp_output */
- if (tp->snd_awnd < tp->snd_cwnd)
- tp->t_flags |= TF_NEEDOUTPUT;
-#else
- tp->snd_cwnd += tp->t_maxseg;
- tp->t_flags |= TF_NEEDOUTPUT;
-#endif /* TCP_FACK */
- } else {
- /* Out of fast recovery */
- tp->snd_cwnd = tp->snd_ssthresh;
- if (tcp_seq_subtract(tp->snd_max,
- th->th_ack) < tp->snd_ssthresh)
- tp->snd_cwnd =
- tcp_seq_subtract(tp->snd_max,
- th->th_ack);
- tp->t_dupacks = 0;
-#ifdef TCP_FACK
- if (SEQ_GT(th->th_ack, tp->snd_fack))
- tp->snd_fack = th->th_ack;
-#endif
- }
- }
- } else {
- if (tp->t_dupacks >= tcprexmtthresh &&
- !tcp_newreno(tp, th)) {
+ if (tp->t_dupacks >= tcprexmtthresh) {
+ if (SEQ_LT(th->th_ack, tp->snd_last)) {
+ if (tp->sack_enable)
+ tcp_sack_partialack(tp, th);
+ else
+ tcp_newreno_partialack(tp, th);
+ } else {
/* Out of fast recovery */
tp->snd_cwnd = tp->snd_ssthresh;
if (tcp_seq_subtract(tp->snd_max, th->th_ack) <
tp->snd_ssthresh)
tp->snd_cwnd =
tcp_seq_subtract(tp->snd_max,
th->th_ack);
tp->t_dupacks = 0;
+#ifdef TCP_FACK
+ if (tp->sack_enable &&
+ SEQ_GT(th->th_ack, tp->snd_fack))
+ tp->snd_fack = th->th_ack;
+#endif
}
- }
- if (tp->t_dupacks < tcprexmtthresh)
+ } else {
+ /*
+* Reset the duplicate ACK counter if we
+* were not in fast recovery.
+*/
tp->t_dupacks = 0;
+ }
if (SEQ_GT(th->th_ack, tp->snd_max)) {
tcpstat_inc(tcps_rcvacktoomuch);
goto dropafterack_ratelim;
}
acked = th->th_ack - tp->snd_una;
@@ -2703,36 +2689,38 @@ tcp_clean_sackreport(struct tcpcb *tp)
tp->sackblks[i].start = tp->sackblks[i].end=0;
}
/*
- * Checks for partial ack. If partial ack arrives, turn off retransmission
- * timer, deflate the window, do not clear tp->t_dupacks, and return 1.
- * If the ack advances at least to tp->snd_last, return 0.
+ * Partial ack handling within a sack recovery episode. When a partial ack
+
Re: Refactor TCP partial ACK handling
On 21/10/17(Sat) 15:17, Mike Belopuhov wrote:
> On Fri, Oct 20, 2017 at 22:59 +0200, Klemens Nanni wrote:
> > The comments for both void tcp_{sack,newreno}_partialack() still mention
> > tp->snd_last and return value bits.
> >
>
> Good eyes! It made me spot a mistake I made by folding two lines
> into an incorrect ifdef in tcp_sack_partialack. I expected it to
> say "ifdef TCP_FACK" while it says "ifNdef". The adjusted comment
> didn't make sense and I found the bug.
Could you send the full/fixed diff? And what about TCP_FACK? It is
disabled by default, is there a point in keeping it?
> diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
> index 45aafee0d05..d5de9cb2407 100644
> --- sys/netinet/tcp_input.c
> +++ sys/netinet/tcp_input.c
> @@ -2690,13 +2690,13 @@ tcp_clean_sackreport(struct tcpcb *tp)
> tp->sackblks[i].start = tp->sackblks[i].end=0;
>
> }
>
> /*
> - * Checks for partial ack. If partial ack arrives, turn off retransmission
> - * timer, deflate the window, do not clear tp->t_dupacks, and return 1.
> - * If the ack advances at least to tp->snd_last, return 0.
> + * Partial ack handling within a sack recovery episode. When a partial ack
> + * arrives, turn off retransmission timer, deflate the window, do not clear
> + * tp->t_dupacks.
> */
> void
> tcp_sack_partialack(struct tcpcb *tp, struct tcphdr *th)
> {
> /* Turn off retx. timer (will start again next segment) */
> @@ -2711,16 +2711,16 @@ tcp_sack_partialack(struct tcpcb *tp, struct tcphdr
> *th)
> if (tp->snd_cwnd > (th->th_ack - tp->snd_una)) {
> tp->snd_cwnd -= th->th_ack - tp->snd_una;
> tp->snd_cwnd += tp->t_maxseg;
> } else
> tp->snd_cwnd = tp->t_maxseg;
> + tp->snd_cwnd += tp->t_maxseg;
> + tp->t_flags |= TF_NEEDOUTPUT;
> +#else
> /* Force call to tcp_output */
> if (tp->snd_awnd < tp->snd_cwnd)
> tp->t_flags |= TF_NEEDOUTPUT;
> -#else
> - tp->snd_cwnd += tp->t_maxseg;
> - tp->t_flags |= TF_NEEDOUTPUT;
> #endif
> }
>
> /*
> * Pull out of band byte out of a segment so
> @@ -3078,14 +3078,14 @@ tcp_mss_update(struct tcpcb *tp)
> }
>
> }
>
> /*
> - * Checks for partial ack. If partial ack arrives, force the retransmission
> - * of the next unacknowledged segment, do not clear tp->t_dupacks, and return
> - * 1. By setting snd_nxt to ti_ack, this forces retransmission timer to
> - * be started again. If the ack advances at least to tp->snd_last, return 0.
> + * When a partial ack arrives, force the retransmission of the
> + * next unacknowledged segment. Do not clear tp->t_dupacks.
> + * By setting snd_nxt to ti_ack, this forces retransmission timer
> + * to be started again.
> */
> void
> tcp_newreno_partialack(struct tcpcb *tp, struct tcphdr *th)
> {
> /*
>
Re: Refactor TCP partial ACK handling
On Fri, Oct 20, 2017 at 22:59 +0200, Klemens Nanni wrote:
> The comments for both void tcp_{sack,newreno}_partialack() still mention
> tp->snd_last and return value bits.
>
Good eyes! It made me spot a mistake I made by folding two lines
into an incorrect ifdef in tcp_sack_partialack. I expected it to
say "ifdef TCP_FACK" while it says "ifNdef". The adjusted comment
didn't make sense and I found the bug.
diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
index 45aafee0d05..d5de9cb2407 100644
--- sys/netinet/tcp_input.c
+++ sys/netinet/tcp_input.c
@@ -2690,13 +2690,13 @@ tcp_clean_sackreport(struct tcpcb *tp)
tp->sackblks[i].start = tp->sackblks[i].end=0;
}
/*
- * Checks for partial ack. If partial ack arrives, turn off retransmission
- * timer, deflate the window, do not clear tp->t_dupacks, and return 1.
- * If the ack advances at least to tp->snd_last, return 0.
+ * Partial ack handling within a sack recovery episode. When a partial ack
+ * arrives, turn off retransmission timer, deflate the window, do not clear
+ * tp->t_dupacks.
*/
void
tcp_sack_partialack(struct tcpcb *tp, struct tcphdr *th)
{
/* Turn off retx. timer (will start again next segment) */
@@ -2711,16 +2711,16 @@ tcp_sack_partialack(struct tcpcb *tp, struct tcphdr *th)
if (tp->snd_cwnd > (th->th_ack - tp->snd_una)) {
tp->snd_cwnd -= th->th_ack - tp->snd_una;
tp->snd_cwnd += tp->t_maxseg;
} else
tp->snd_cwnd = tp->t_maxseg;
+ tp->snd_cwnd += tp->t_maxseg;
+ tp->t_flags |= TF_NEEDOUTPUT;
+#else
/* Force call to tcp_output */
if (tp->snd_awnd < tp->snd_cwnd)
tp->t_flags |= TF_NEEDOUTPUT;
-#else
- tp->snd_cwnd += tp->t_maxseg;
- tp->t_flags |= TF_NEEDOUTPUT;
#endif
}
/*
* Pull out of band byte out of a segment so
@@ -3078,14 +3078,14 @@ tcp_mss_update(struct tcpcb *tp)
}
}
/*
- * Checks for partial ack. If partial ack arrives, force the retransmission
- * of the next unacknowledged segment, do not clear tp->t_dupacks, and return
- * 1. By setting snd_nxt to ti_ack, this forces retransmission timer to
- * be started again. If the ack advances at least to tp->snd_last, return 0.
+ * When a partial ack arrives, force the retransmission of the
+ * next unacknowledged segment. Do not clear tp->t_dupacks.
+ * By setting snd_nxt to ti_ack, this forces retransmission timer
+ * to be started again.
*/
void
tcp_newreno_partialack(struct tcpcb *tp, struct tcphdr *th)
{
/*
Re: Refactor TCP partial ACK handling
On Fri, Oct 20, 2017 at 09:07:20PM +0200, Mike Belopuhov wrote:
> This is a small and not intrusive refactoring of partial ACK handling
> but it certainly doesn't look like one. It's intended to be applied
> after the TCP SACK diff that I've sent earlier and basically moves the
> conditional (SEQ_LT(th->th_ack, tp->snd_last)) out of tcp_sack_partialack
> and tcp_newreno into the tcp_input itself making these two functions just
> do the work and let tcp_input make decisions. Here's how it looks after
> refactoring:
>
> if (tp->t_dupacks >= tcprexmtthresh) {
> if (SEQ_LT(th->th_ack, tp->snd_last)) {
> if (tp->sack_enable)
> tcp_sack_partialack(tp, th);
> else
> tcp_newreno_partialack(tp, th);
> } else {
> /* Out of fast recovery */
> tp->snd_cwnd = tp->snd_ssthresh;
> if (tcp_seq_subtract(tp->snd_max, th->th_ack) <
> tp->snd_ssthresh)
> tp->snd_cwnd =
> tcp_seq_subtract(tp->snd_max,
> th->th_ack);
> tp->t_dupacks = 0;
> #ifdef TCP_FACK
> if (tp->sack_enable &&
> SEQ_GT(th->th_ack, tp->snd_fack))
> tp->snd_fack = th->th_ack;
> #endif
> }
> } else {
> /*
>* Reset the duplicate ACK counter if we
>* were not in fast recovery.
>*/
> tp->t_dupacks = 0;
> }
>
> This allows to consolidate the "out of fast recovery" branch currently
> repeated twice as well as show how tcp_sack_partialack and tcp_newreno
> interact without extra clutter. The true branch of the old condition
> "if (tcp_sack_partialack(tp, th))" gets integrated into the function
> tcp_sack_partialack itself and tcp_newreno is renamed for consistency.
>
> The diff also hooks up the "if (tp->t_dupacks < tcprexmtthresh)" branch
> to this "main if" since t_dupacks is ether greater then tcprexmtthresh
> or not.
>
> In the end there's no (intentional) logic change at all, but gained
> clarity is quite substantial as noticed by FreeBSD folks as well.
> Consolidating the 'out of fast recovery' branch is also beneficial for
> later work.
>
> OK?
>
> diff --git sys/netinet/tcp_input.c sys/netinet/tcp_input.c
> index 9951923bbdb..84cdb35f048 100644
> --- sys/netinet/tcp_input.c
> +++ sys/netinet/tcp_input.c
> @@ -1664,52 +1664,38 @@ trimthenstep6:
> }
> /*
>* If the congestion window was inflated to account
>* for the other side's cached packets, retract it.
>*/
> - if (tp->sack_enable) {
> - if (tp->t_dupacks >= tcprexmtthresh) {
> - /* Check for a partial ACK */
> - if (tcp_sack_partialack(tp, th)) {
> -#ifdef TCP_FACK
> - /* Force call to tcp_output */
> - if (tp->snd_awnd < tp->snd_cwnd)
> - tp->t_flags |= TF_NEEDOUTPUT;
> -#else
> - tp->snd_cwnd += tp->t_maxseg;
> - tp->t_flags |= TF_NEEDOUTPUT;
> -#endif /* TCP_FACK */
> - } else {
> - /* Out of fast recovery */
> - tp->snd_cwnd = tp->snd_ssthresh;
> - if (tcp_seq_subtract(tp->snd_max,
> - th->th_ack) < tp->snd_ssthresh)
> - tp->snd_cwnd =
> -tcp_seq_subtract(tp->snd_max,
> -th->th_ack);
> - tp->t_dupacks = 0;
> -#ifdef TCP_FACK
> - if (SEQ_GT(th->th_ack, tp->snd_fack))
> - tp->snd_fack = th->th_ack;
> -#endif
> - }
> - }
> - } else {
> - if (tp->t_dupacks >= tcprexmtthresh &&
> - !tcp_newreno(tp, th)) {
> + if (tp->t_dupacks >= tcprexmtthresh) {
> + if (SEQ_LT(th->th_ack, tp->snd_last)) {
> + if (tp->sack_enable)
> + tcp_sack_partialack(tp, th);
> + else
> + tcp_newreno_partialack(tp, th);
> + } else {
> /* Out of fast recovery */
> tp->snd_cwnd = tp->snd_ssthresh;
>
