Bug 1405 - RttEstimator improvements
RttEstimator improvements
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: tcp
pre-release
All All
: P5 normal
Assigned To: Brian Swenson
:
Depends on:
Blocks: 1412 2041
  Show dependency treegraph
 
Reported: 2012-04-04 13:16 EDT by Tom Henderson
Modified: 2015-01-30 17:09 EST (History)
4 users (show)

See Also:


Attachments
Daniel's patch from bug 1399 for RFC 6298 behavior (4.57 KB, patch)
2012-05-30 13:29 EDT, Tom Henderson
Details | Diff
Huge patch (32.35 KB, patch)
2015-01-25 09:12 EST, Tommaso Pecorella
Details | Diff
modifications to Tommaso's Huge patch (44.97 KB, patch)
2015-01-29 12:19 EST, Tom Henderson
Details | Diff
Mods to the mods to my patch (44.67 KB, patch)
2015-01-30 01:41 EST, Tommaso Pecorella
Details | Diff
Mods to the mods to my patch (fix) (44.68 KB, patch)
2015-01-30 01:58 EST, Tommaso Pecorella
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2012-04-04 13:16:55 EDT
I'm not confident in the results of this class; a unit test is needed.
Comment 1 Tom Henderson 2012-05-30 13:29:26 EDT
Created attachment 1408 [details]
Daniel's patch from bug 1399 for RFC 6298 behavior
Comment 2 Tom Henderson 2012-05-30 13:36:46 EDT
I moved Daniel's patch from bug 1399 here.  It implements RFC 6298 behavior (slightly different than the MeanDeviation algorithm currently implemented).

Another aspect that must be considered is whether the TCP or the RttEstimator class maintains the state for the backoff.  In the current design, RttEstimator is managing all of this state, but in Daniel's patch, he moves the state for managing backoff to the TcpSocketBase class.

I'm in favor of Daniel's proposal to make TCP manage the backoff state and just have RttEstimator maintain actual estimates, but this would require removing some state variables and the "multiplier" API from RttEstimator and should be considered again once we are not close to a release deadline.  The reason that I am in favor of this is the separate proposal (bug 1412) that wants to reuse RttEstimator for other protocols.
Comment 3 Tom Henderson 2015-01-24 10:46:14 EST
I'm going to put this in the list of issues to resolve for ns-3.23 release.
Comment 4 Tommaso Pecorella 2015-01-25 03:10:26 EST
This patch isn't working as intended.
The rto value is not multiplied as it should in case of "normal" retransmissions.
I'm working on a new patch.
Comment 5 Tommaso Pecorella 2015-01-25 09:12:29 EST
Created attachment 1955 [details]
Huge patch

I'll attach the patch here, but this is a cumulative patch, fixing this bug, bug #2041 and another undiscovered bug.

For the records, the undiscovered bug is: RTO should increase exponentially, while it was increasing linearly (in case of consecutive lost packets).

Sorry, but the patch is huge and can't really be split in sub-components.

What's been done:
1) update the RTT estimation to RFC 6298 values, including *two* smoothing factors for mean and variance
2) remove RTO calculation from RttEstimator and place it into TcpSocketBase
3) move to TcpSocketBase the RTT sample calc (and move RttHistory there as well)
4) remove RTO multiplier and use a different approach
5) add limits to the RTO, as suggested by RFC 6298
6) something more I can't remember.

Please test is as much as you can. The Tcp performances are expected to vary slightly, and we'll need to re-generate some tracefiles.
Comment 6 Tom Henderson 2015-01-29 12:19:03 EST
Created attachment 1957 [details]
modifications to Tommaso's Huge patch

I made a number of changes to Tommaso's huge patch, driven by a rewrite of the existing unit test, which now is validating the arithmetic for selected (rather than random) values.

The main change in the computation is to use integer arithmetic when the values of alpha and beta support it (since time is maintained as an integer) and fall back to floating point arithmetic when integer can't be supported.

Other small changes:

* s/GetEstimateVariation/GetVariation
* add a GetNSamples() method
* more Doxygen
* check bounds on Alpha and Beta attributes
* cleanup includes and forward declarations

This does not yet address bug 1412 (move rtt-estimator to network module) but such a change should be trivial.

This does not yet fully address bug 2041 (does not test RTO computations in any way).
Comment 7 Tommaso Pecorella 2015-01-30 01:41:36 EST
Created attachment 1959 [details]
Mods to the mods to my patch

Just some small improvements to Tom's modifications.
1) Use Abs, Min and Max (the ones defined in Time class) where appropriate
2) fix in the test (wrong function name)
3) use an optimized version of the floating-point update formulas.

The last point is because
X = (1-a) X' + a m
is equivalent to
X += a (m - X')

For the records, not only this is important in hardware implementation (1 mul less), but it is also more precise (when a is very low).
Comment 8 Tommaso Pecorella 2015-01-30 01:58:35 EST
Created attachment 1960 [details]
Mods to the mods to my patch (fix)

Forgot to refresh the diff.
Comment 9 Tom Henderson 2015-01-30 09:56:12 EST
(In reply to Tommaso Pecorella from comment #7)
> Created attachment 1959 [details]
> Mods to the mods to my patch
> 
> Just some small improvements to Tom's modifications.
> 1) Use Abs, Min and Max (the ones defined in Time class) where appropriate
> 2) fix in the test (wrong function name)
> 3) use an optimized version of the floating-point update formulas.
> 
> The last point is because
> X = (1-a) X' + a m
> is equivalent to
> X += a (m - X')
> 
> For the records, not only this is important in hardware implementation (1
> mul less), but it is also more precise (when a is very low).


Thanks, I'm fine with your improvements to this patch.
Comment 10 Tommaso Pecorella 2015-01-30 17:09:44 EST
changeset:   11190:f0458968b67d