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;
> 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;
> @@ -2707,32 +2693,34 @@ tcp_clean_sackreport(struct tcpcb *tp)
> /*
> * 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.
> */
> -int
> +void
> tcp_sack_partialack(struct tcpcb *tp, struct tcphdr *th)
> {
> - if (SEQ_LT(th->th_ack, tp->snd_last)) {
> - /* Turn off retx. timer (will start again next segment) */
> - TCP_TIMER_DISARM(tp, TCPT_REXMT);
> - tp->t_rtttime = 0;
> + /* Turn off retx. timer (will start again next segment) */
> + TCP_TIMER_DISARM(tp, TCPT_REXMT);
> + tp->t_rtttime = 0;
> #ifndef TCP_FACK
> - /*
> - * Partial window deflation. This statement relies on the
> - * fact that tp->snd_una has not been updated yet. In FACK
> - * hold snd_cwnd constant during fast recovery.
> - */
> - 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;
> + /*
> + * Partial window deflation. This statement relies on the
> + * fact that tp->snd_una has not been updated yet. In FACK
> + * hold snd_cwnd constant during fast recovery.
> + */
> + 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;
> + /* 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
> - return (1);
> - }
> - return (0);
> }
>
> /*
> * Pull out of band byte out of a segment so
> * it doesn't appear in the user's data queue.
> @@ -3094,47 +3082,42 @@ 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.
> */
> -int
> -tcp_newreno(struct tcpcb *tp, struct tcphdr *th)
> +void
> +tcp_newreno_partialack(struct tcpcb *tp, struct tcphdr *th)
> {
> - if (SEQ_LT(th->th_ack, tp->snd_last)) {
> - /*
> - * snd_una has not been updated and the socket send buffer
> - * not yet drained of the acked data, so we have to leave
> - * snd_una as it was to get the correct data offset in
> - * tcp_output().
> - */
> - tcp_seq onxt = tp->snd_nxt;
> - u_long ocwnd = tp->snd_cwnd;
> - TCP_TIMER_DISARM(tp, TCPT_REXMT);
> - tp->t_rtttime = 0;
> - tp->snd_nxt = th->th_ack;
> - /*
> - * Set snd_cwnd to one segment beyond acknowledged offset
> - * (tp->snd_una not yet updated when this function is called)
> - */
> - tp->snd_cwnd = tp->t_maxseg + (th->th_ack - tp->snd_una);
> - (void) tcp_output(tp);
> - tp->snd_cwnd = ocwnd;
> - if (SEQ_GT(onxt, tp->snd_nxt))
> - tp->snd_nxt = onxt;
> - /*
> - * Partial window deflation. Relies on fact that tp->snd_una
> - * not updated yet.
> - */
> - if (tp->snd_cwnd > th->th_ack - tp->snd_una)
> - tp->snd_cwnd -= th->th_ack - tp->snd_una;
> - else
> - tp->snd_cwnd = 0;
> - tp->snd_cwnd += tp->t_maxseg;
> -
> - return 1;
> - }
> - return 0;
> + /*
> + * snd_una has not been updated and the socket send buffer
> + * not yet drained of the acked data, so we have to leave
> + * snd_una as it was to get the correct data offset in
> + * tcp_output().
> + */
> + tcp_seq onxt = tp->snd_nxt;
> + u_long ocwnd = tp->snd_cwnd;
> + TCP_TIMER_DISARM(tp, TCPT_REXMT);
> + tp->t_rtttime = 0;
> + tp->snd_nxt = th->th_ack;
> + /*
> + * Set snd_cwnd to one segment beyond acknowledged offset
> + * (tp->snd_una not yet updated when this function is called)
> + */
> + tp->snd_cwnd = tp->t_maxseg + (th->th_ack - tp->snd_una);
> + (void) tcp_output(tp);
> + tp->snd_cwnd = ocwnd;
> + if (SEQ_GT(onxt, tp->snd_nxt))
> + tp->snd_nxt = onxt;
> + /*
> + * Partial window deflation. Relies on fact that tp->snd_una
> + * not updated yet.
> + */
> + if (tp->snd_cwnd > th->th_ack - tp->snd_una)
> + tp->snd_cwnd -= th->th_ack - tp->snd_una;
> + else
> + tp->snd_cwnd = 0;
> + tp->snd_cwnd += tp->t_maxseg;
> }
>
> int
> tcp_mss_adv(struct mbuf *m, int af)
> {
> diff --git sys/netinet/tcp_var.h sys/netinet/tcp_var.h
> index bc2b77d188b..c44cf2bcaa3 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);
> void tcp_del_sackholes(struct tcpcb *, struct tcphdr *);
> void tcp_clean_sackreport(struct tcpcb *tp);
> void tcp_sack_adjust(struct tcpcb *tp);
> struct sackhole *
> tcp_sack_output(struct tcpcb *tp);
> -int tcp_sack_partialack(struct tcpcb *, struct tcphdr *);
> +void tcp_sack_partialack(struct tcpcb *, struct tcphdr *);
> +void tcp_newreno_partialack(struct tcpcb *, struct tcphdr *);
> #ifdef DEBUG
> void tcp_print_holes(struct tcpcb *tp);
> #endif
> -int tcp_newreno(struct tcpcb *, struct tcphdr *);
> u_long tcp_seq_subtract(u_long, u_long );
> #ifdef TCP_SIGNATURE
> int tcp_signature_apply(caddr_t, caddr_t, unsigned int);
> int tcp_signature(struct tdb *, int, struct mbuf *, struct tcphdr *,
> int, int, char *);
>
The comments for both void tcp_{sack,newreno}_partialack() still mention
tp->snd_last and return value bits.
