| Message ID | alpine.DEB.2.00.0901071009000.6102@rhea |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 7 Jan 2009, Arnd Hannemann wrote: > I noticed Good that somebody else is looking TCP code besides me... :-) > that in tcp_mark_lost_retrans the for-loop is only entered > if tcp_is_fack(tp) evaluates to true: > > if (!tcp_is_fack(tp) || !tp->retrans_out || > !after(received_upto, tp->lost_retrans_low) || > icsk->icsk_ca_state != TCP_CA_Recovery) > return; > > Therefore the following check in the for-loop seems to be redundant, > because it always evaluates to true: > > (tcp_is_fack(tp) || > !before(received_upto, > ack_seq + tp->reordering * tp->mss_cache)) > > Did I miss something? It was just a left over from the RFC3517 SACK addition which added that !tcp_is_fack(tp) there above. ...It would have been nice to have similar lost rexmit feature without FACK as well but calculating that wasn't trivial (or I didn't find that too trivial) and could end up being extremely expensive in case of large holes. (So I also left it there as sort of reminder). On the second thought, it would be possible to count skbs we pass while walking from the beginning and use that a remaining_sacked counter to get rid of all heurestics too and base the counting only on sacked stuff which aligns with the spirit of rfc3517 much better than sacked+holes used by fack. > Best regards, > Arnd Hannemann > > From: Arnd Hannemann <hannemann@nets.rwth-aachen.de> > > Because the for loop is only executed for FACK-enabled flows remove > redundant checks within the loop. > > Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de> > --- > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 99b7ecb..cd8b4bd 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -1178,10 +1178,7 @@ static void tcp_mark_lost_retrans(struct sock *sk) > if (!(TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_RETRANS)) > continue; > > - if (after(received_upto, ack_seq) && > - (tcp_is_fack(tp) || > - !before(received_upto, > - ack_seq + tp->reordering * tp->mss_cache))) { > + if (after(received_upto, ack_seq)) { > TCP_SKB_CB(skb)->sacked &= ~TCPCB_SACKED_RETRANS; > tp->retrans_out -= tcp_skb_pcount(skb);
On Wed, 7 Jan 2009, Ilpo Järvinen wrote: > On Wed, 7 Jan 2009, Arnd Hannemann wrote: > > > I noticed > > Good that somebody else is looking TCP code besides me... :-) > > > that in tcp_mark_lost_retrans the for-loop is only entered > > if tcp_is_fack(tp) evaluates to true: > > > > if (!tcp_is_fack(tp) || !tp->retrans_out || > > !after(received_upto, tp->lost_retrans_low) || > > icsk->icsk_ca_state != TCP_CA_Recovery) > > return; > > > > Therefore the following check in the for-loop seems to be redundant, > > because it always evaluates to true: > > > > (tcp_is_fack(tp) || > > !before(received_upto, > > ack_seq + tp->reordering * tp->mss_cache)) > > > > Did I miss something? > > It was just a left over from the RFC3517 SACK addition which added that > !tcp_is_fack(tp) there above. ...It would have been nice to have similar > lost rexmit feature without FACK as well but calculating that wasn't > trivial (or I didn't find that too trivial) and could end up being > extremely expensive in case of large holes. (So I also left it there as > sort of reminder). > > On the second thought, it would be possible to count skbs we pass while > walking from the beginning and use that a remaining_sacked counter > to get rid of all heurestics too and base the counting only on sacked > stuff which aligns with the spirit of rfc3517 much better than > sacked+holes used by fack. Nah, tried to do that that wasn't working nicely either... Since there is a need to know how many sack blocks reside between ack_seq and received_upto, not the number of sack blocks between skb and received_upto...
Ilpo Järvinen schrieb: > On Wed, 7 Jan 2009, Arnd Hannemann wrote: > > >> I noticed >> > > Good that somebody else is looking TCP code besides me... :-) > Well I try hard... ;-) > >> that in tcp_mark_lost_retrans the for-loop is only entered >> if tcp_is_fack(tp) evaluates to true: >> >> if (!tcp_is_fack(tp) || !tp->retrans_out || >> !after(received_upto, tp->lost_retrans_low) || >> icsk->icsk_ca_state != TCP_CA_Recovery) >> return; >> >> Therefore the following check in the for-loop seems to be redundant, >> because it always evaluates to true: >> >> (tcp_is_fack(tp) || >> !before(received_upto, >> ack_seq + tp->reordering * tp->mss_cache)) >> >> Did I miss something? >> > > It was just a left over from the RFC3517 SACK addition which added that > !tcp_is_fack(tp) there above. ...It would have been nice to have similar > lost rexmit feature without FACK as well but calculating that wasn't > trivial (or I didn't find that too trivial) and could end up being > extremely expensive in case of large holes. (So I also left it there as > sort of reminder). > Perhaps it would be better to let the comments reflect what you just said and remove the redundant check anyway to reduce the dead code a newcomer has to understand ;-) I would have included a patch for the comments, but as you have a deeper understanding of the code it would probably be better if you can do it. Best regards, Arnd -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 7 Jan 2009, Arnd Hannemann wrote: > Perhaps it would be better to let the comments reflect > what you just said and remove the redundant check > anyway to reduce the dead code a newcomer has to understand ;-) > I would have included a patch for the comments, but as you have a > deeper understanding of the code it would probably > be better if you can do it. I have a patch for this in a local tree and will submit it among other TCP stuff (later on).
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 99b7ecb..cd8b4bd 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -1178,10 +1178,7 @@ static void tcp_mark_lost_retrans(struct sock *sk) if (!(TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_RETRANS)) continue; - if (after(received_upto, ack_seq) && - (tcp_is_fack(tp) || - !before(received_upto, - ack_seq + tp->reordering * tp->mss_cache))) { + if (after(received_upto, ack_seq)) { TCP_SKB_CB(skb)->sacked &= ~TCPCB_SACKED_RETRANS; tp->retrans_out -= tcp_skb_pcount(skb);
Hi, I noticed that in tcp_mark_lost_retrans the for-loop is only entered if tcp_is_fack(tp) evaluates to true: if (!tcp_is_fack(tp) || !tp->retrans_out || !after(received_upto, tp->lost_retrans_low) || icsk->icsk_ca_state != TCP_CA_Recovery) return; Therefore the following check in the for-loop seems to be redundant, because it always evaluates to true: (tcp_is_fack(tp) || !before(received_upto, ack_seq + tp->reordering * tp->mss_cache)) Did I miss something? Best regards, Arnd Hannemann From: Arnd Hannemann <hannemann@nets.rwth-aachen.de> Because the for loop is only executed for FACK-enabled flows remove redundant checks within the loop. Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de> --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html