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) > 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. > */ > -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; > + 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; > #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. > @@ -3089,52 +3077,48 @@ 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. > */ > -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; > + /* > + * 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; > > - return 1; > - } > - return 0; > + 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 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); > 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 *); 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. > 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 *); >