Re: [PATCH-2.4] Fix divide by 0 in vegas_cong_avoid()

["Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2442 bytes --]

On Wed, 30 May 2007, Stephen Hemminger wrote:

> On Wed, 30 May 2007 10:49:54 +0300 (EEST)
> "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote:
> 
> > I think the code did a right thing before your api merge, since it called 
> > rtt callback only if FLAG_RETRANS_DATA_ACKED was not set (and pkts_acked 
> > always), i.e., when TCP cannot know if it was the rexmission that 
> > triggered the cumulative ACK or any original transmission, no new RTT 
> > measurement should be made. Consider also the fact that, there might be a 
> > non rexmitted skb after rexmitted one, which Lior's patch doesn't do 
> > right way at all since the FLAG_DATA_ACKED would again get set (no 
> > div-by-zero would have occured then but an unreliable timestamp would
> > have been givento vegas in 2.4).
> > 
> > The number of packets that got cumulatively acked is never a right metric 
> > to measure whether the cumulative ACK originated from rexmitted skbs or 
> > not. ...Perhaps the FLAG_RETRANS_DATA_ACKED flag needs to be passed 
> > somehow to cc modules?
> 
> What about the mixed case where some retransmitted data and new
> data was covered by one ack. 

Without timestamps TCP cannot do anything better than consider a
cumulative ACK that includes any ever retransmitted segment as
inaccurate timestamp. Like in tcp_ack_no_tstamp:

        if (flag & FLAG_RETRANS_DATA_ACKED)
                return;

...I think this same applies to cc modules as well. With timestamps 
things are another beast.

> I would rather keep the merge, but think about the cases more
> carefully.

IMHO there's no problem with keeping the merge, you just need to add 
another parameter to pkts_acked (or by some other means indicate it) to 
distinguish RTT measurement with accurate timestamp from inaccurate ones. 
Maybe the first step (to 2.6.22 to avoid new bugs etc.) should be just to 
give the flag (for some funny reason, it's called "acked" in 
tcp_clean_rtx_queue) or use a single arg with 0/1 value fo depending on 
FLAG_RETRANS_DATA_ACKED and check that in the merged pkts_acked in the 
cases that were using the old rtt callback. It would allow retaining the 
previous (correct) functionality as it was before the API merge. That 
would prevent potential regressions in cc modules due to this merge. 

...Then perhaps come up with a solution that takes advantage of timestamps 
more aggressively to 2.6.23 like the core rtt estimator already does. 


-- 
 i.