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;
>
Refactor TCP partial ACK handling
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;
if (tcp_seq_subtract(tp->snd_max, th->th_ack) <
tp->snd_ssthresh)
