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)
> {
> /*
>
