Bug 1831

Summary: TcpSocket SlowStartThreshold is not a TraceSource
Product: ns-3 Reporter: Tommaso Pecorella <tommaso.pecorella>
Component: internetAssignee: Tommaso Pecorella <tommaso.pecorella>
Status: RESOLVED FIXED    
Severity: normal CC: bswenson3, ns-bugs, tomh
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
Attachments: patch
new patch...
tcp-variants-comparison improvements
Split patch

Description Tommaso Pecorella 2014-01-11 08:49:59 EST
SlowStartThreshold should be plotted by examples/tcp/tcp-variants-comparison.cc, but it's not a trace source.

We could fix the example, but it's better to add it as a trace source, isn't it ?

By the way, why CongestionWindow is defined in every TCP flavor and not in the base class ?
Why do we have TcpSocket -> TcpSocketBase -> TcpWhatever ?

Mysteries…

T.
Comment 1 Tom Henderson 2014-02-14 16:14:19 EST
(In reply to Tommaso Pecorella from comment #0)
> SlowStartThreshold should be plotted by
> examples/tcp/tcp-variants-comparison.cc, but it's not a trace source.
> 
> We could fix the example, but it's better to add it as a trace source, isn't
> it ?

Agree, there doesn't seem to be a reason not to.

> 
> By the way, why CongestionWindow is defined in every TCP flavor and not in
> the base class ?
> Why do we have TcpSocket -> TcpSocketBase -> TcpWhatever ?
> 
> Mysteries…

I believe that it is due to historical reasons; the abstract base class TcpSocket is used by both NSC TCp and native ns-3 TCP.  TcpSocketBase is used by TcpRfc793 which doesn't use congestion window.  

I don't see much harm, however, in moving those variables to TcpSocketBase if it avoids confusion and replicated code.
Comment 2 Tommaso Pecorella 2014-02-15 09:44:02 EST
Created attachment 1781 [details]
patch

TraceSource added and tweaked the example to actually work.

I didn't push it (yet) because I'm not sure about one thing.

It is ok to have BOTH an Attribute and a TraceSource with the same name ?
I mean, the compiler isn't complaining. The example works. But... is it ok ?

T.
Comment 3 Brian Swenson 2014-04-29 13:05:24 EDT
I talked to Tom and he thinks, and I agree that its a bad idea to have a trace source named the same as an attribute.  If nothing else it could be confusing.  I guess ideally the attribute should be renamed InitialSsthresh or something similar to follow how the cwnd is setup however that would lead a breaking change.  

I agree that it makes more sense to have cwnd and ssthresh in TcpSocketBase.  I assume the reason why it wasn't done is because TcpRfc793 doesn't use them and it also inherits from TcpSocketBase.  That said TcpRfc793 does have overloads for SetSSThresh, GetSSThresh, SetInitialCwnd, and GetInitialCwnd however that is because they're pure virtual in TcpSocketBase.  So moving cwnd and ssThresh wouldn't make it any more confusing than it already is.
Comment 4 Tommaso Pecorella 2014-04-29 16:31:47 EDT
Hi,

having a trace source and an attribute with the same name was my main concern too.

About renaming the attribute, I don't fear it. The change:
- is easy to perform in any script,
- can be done with a single "sed" line,
- the error ns-3 would issue is very explicative (no such an attribute).

As a consequence, I'd change the attribute name. Also because it would clarify the attribute meaning.

If we want to avoid that, please suggest a name for the trace source. I'm very bad with names.

About moving cwnd and ssthresh to TcpSocketBase, it was just a philosophical question. I'm not proposing to move them, as this would definitely be a pain and wouldn't bring any real benefit.


(In reply to Brian Swenson from comment #3)
> I talked to Tom and he thinks, and I agree that its a bad idea to have a
> trace source named the same as an attribute.  If nothing else it could be
> confusing.  I guess ideally the attribute should be renamed InitialSsthresh
> or something similar to follow how the cwnd is setup however that would lead
> a breaking change.  
> 
> I agree that it makes more sense to have cwnd and ssthresh in TcpSocketBase.
> I assume the reason why it wasn't done is because TcpRfc793 doesn't use them
> and it also inherits from TcpSocketBase.  That said TcpRfc793 does have
> overloads for SetSSThresh, GetSSThresh, SetInitialCwnd, and GetInitialCwnd
> however that is because they're pure virtual in TcpSocketBase.  So moving
> cwnd and ssThresh wouldn't make it any more confusing than it already is.
Comment 5 Tom Henderson 2014-04-30 00:48:27 EDT
(In reply to Brian Swenson from comment #3)
> I talked to Tom and he thinks, and I agree that its a bad idea to have a
> trace source named the same as an attribute.  If nothing else it could be
> confusing.  I guess ideally the attribute should be renamed InitialSsthresh
> or something similar to follow how the cwnd is setup however that would lead
> a breaking change.  

I agree, it is more than a rename, it should probably be changed to work similar to InitialCwnd as in not being settable once the connection opens.

Where would SlowStartThreshold be implemented as a trace source?  In every TCP variant such as CongestionWindow, or in the base?

I'm a bit leery of this late stage change; it might be better to wait until after the release when the changes can be thought through more and tested.

> 
> I agree that it makes more sense to have cwnd and ssthresh in TcpSocketBase.
> I assume the reason why it wasn't done is because TcpRfc793 doesn't use them
> and it also inherits from TcpSocketBase.  That said TcpRfc793 does have
> overloads for SetSSThresh, GetSSThresh, SetInitialCwnd, and GetInitialCwnd
> however that is because they're pure virtual in TcpSocketBase.  So moving
> cwnd and ssThresh wouldn't make it any more confusing than it already is.
Comment 6 Tommaso Pecorella 2014-04-30 02:35:19 EDT
I'm ok in making this slip to the next release, if this means that we plan to perform a more consistent change (attribute rename, etc.).

I'm just concerned about the fact that we have an example which *should* show SshThreshold and it isn't.

Anyway, a documented bug is a feature, and this is documented. Let's just not forget.


(In reply to Tom Henderson from comment #5)
> (In reply to Brian Swenson from comment #3)
> > I talked to Tom and he thinks, and I agree that its a bad idea to have a
> > trace source named the same as an attribute.  If nothing else it could be
> > confusing.  I guess ideally the attribute should be renamed InitialSsthresh
> > or something similar to follow how the cwnd is setup however that would lead
> > a breaking change.  
> 
> I agree, it is more than a rename, it should probably be changed to work
> similar to InitialCwnd as in not being settable once the connection opens.
> 
> Where would SlowStartThreshold be implemented as a trace source?  In every
> TCP variant such as CongestionWindow, or in the base?
> 
> I'm a bit leery of this late stage change; it might be better to wait until
> after the release when the changes can be thought through more and tested.
> 
> > 
> > I agree that it makes more sense to have cwnd and ssthresh in TcpSocketBase.
> > I assume the reason why it wasn't done is because TcpRfc793 doesn't use them
> > and it also inherits from TcpSocketBase.  That said TcpRfc793 does have
> > overloads for SetSSThresh, GetSSThresh, SetInitialCwnd, and GetInitialCwnd
> > however that is because they're pure virtual in TcpSocketBase.  So moving
> > cwnd and ssThresh wouldn't make it any more confusing than it already is.
Comment 7 Tommaso Pecorella 2014-08-13 07:05:25 EDT
Created attachment 1863 [details]
new patch...

This new patch adds the following:
1) "SlowStartThreshold" is now a TraceSource
2) "SlowStartThreshold" attribute has been renamed "InitialSlowStartThreshold"
3) "GetSSThresh" and "SetSSThresh" have been renamed "[G,S]etInitialSSThresh"
4) a new protected variable "m_initialSsThresh" has been added.

The m_ssThresh (traced) variable is now set to its initial value in the "InitializeCwnd" functions. The function name could be changed to indicate that it does set more than one parameter to its initial value.

The patch is... intrusive, however it is necessary. Also because there's a long-standing unknown bug being fixed in this way:
a TCP socket closed and repoened would have the (old) m_ssThresh value, possibly resulting in poor performances.

Moreover, the affected functions seems to be never used.

The only (remotely possible) thing to consider is that the old API could allow to change the Slow Start Thresh while the connection was opened. This could be used for some (strange?) reason. If this will be proven to be a necessary feature, we could add some (new) functions, but it's more or less like having a way to modify the cWnd. We don't have an API right now, and I don't think it was the intended behaviour for the ssThreshold functions.

As a side note, the patch also includes some modifications to the tcp-variants-comparison.cc in order to track all the Slow Start and Congestion Window changes. Perhaps even too many, as it does track temporary changes.
Comment 8 Tom Henderson 2014-08-13 09:50:40 EDT
(In reply to Tommaso Pecorella from comment #7)
> Created attachment 1863 [details]
> new patch...
> 
> This new patch adds the following:
> 1) "SlowStartThreshold" is now a TraceSource
> 2) "SlowStartThreshold" attribute has been renamed
> "InitialSlowStartThreshold"
> 3) "GetSSThresh" and "SetSSThresh" have been renamed "[G,S]etInitialSSThresh"
> 4) a new protected variable "m_initialSsThresh" has been added.

I support the internal changes to separate current ssthresh from initial ssthresh, but this patch could be further split and simplified.

- separate out all of the whitespace changes to the example program as a separate patch (or all of the changes to this example into a separate patch)

- there are built-in getters and setters for attribute values so can we just delete all of the custom getters/setters for these?  I suspect there is little need for a custom setter for initial ssthresh (outside of normal attribute and default settings).
Comment 9 Tommaso Pecorella 2014-08-13 11:49:23 EDT
(In reply to Tom Henderson from comment #8)
> (In reply to Tommaso Pecorella from comment #7)
> > Created attachment 1863 [details]
> > new patch...
> > 
> > This new patch adds the following:
> > 1) "SlowStartThreshold" is now a TraceSource
> > 2) "SlowStartThreshold" attribute has been renamed
> > "InitialSlowStartThreshold"
> > 3) "GetSSThresh" and "SetSSThresh" have been renamed "[G,S]etInitialSSThresh"
> > 4) a new protected variable "m_initialSsThresh" has been added.
> 
> I support the internal changes to separate current ssthresh from initial
> ssthresh, but this patch could be further split and simplified.
> 
> - separate out all of the whitespace changes to the example program as a
> separate patch (or all of the changes to this example into a separate patch)
> 
> - there are built-in getters and setters for attribute values so can we just
> delete all of the custom getters/setters for these?  I suspect there is
> little need for a custom setter for initial ssthresh (outside of normal
> attribute and default settings).

I agree about the getters/setters. The actual ones (both the SlowStart and the CWnd ones) are there just to throw an error if they are called when the connection is started.
Since it is now clear that they are for *initial* values, we can safely remove the four functions.
Comment 10 Tommaso Pecorella 2014-08-13 11:57:29 EDT
Created attachment 1864 [details]
tcp-variants-comparison improvements

Various improvements to the script.
Most notably, SSThreshold and cWnd are both printed when one of the two changes.
Since ssThreshold does not change a lot, this change produces better plots.

Other than this... typos, spaces and minor things.
Comment 11 Tommaso Pecorella 2014-08-13 12:01:05 EDT
Created attachment 1865 [details]
Split patch

I did *no* remove the Getters/Setters, because they are virtual functions.

To remove them we should move m_initial{cWnd,ssThr} to the Socket class, and make them protected.

If there are no objections in this, we can simplify the API.

About the (similar) SegmentSize getters and setters, I'd rather leave them as they are. Sooner or later we may want to have dynamic segment sizes.
Comment 12 Tom Henderson 2014-08-13 15:13:43 EDT
(In reply to Tommaso Pecorella from comment #11)
> Created attachment 1865 [details]
> Split patch
> 
> I did *no* remove the Getters/Setters, because they are virtual functions.


Yes, they are also private which I forgot when I replied to you above.  I was concerned about redundant public setters.

> 
> To remove them we should move m_initial{cWnd,ssThr} to the Socket class, and
> make them protected.
> 
> If there are no objections in this, we can simplify the API.

-1 on moving to ns3::Socket

> 
> About the (similar) SegmentSize getters and setters, I'd rather leave them
> as they are. Sooner or later we may want to have dynamic segment sizes.

I am fine with your latest patch(es) as is; thanks for your work on it.

I think that we should also move ssthresh and cwnd trace sources out of the derived classes to TcpSocketBase (or TcpSocket) but let's leave that as a separate issue for now.  Perhaps the bug 1673 fix will allow us to do this in a backward-compatible way.
Comment 13 Tommaso Pecorella 2014-08-13 17:46:51 EDT
Pushed in:
- 10855:7ef081ddfc7f
- 10856:d45187afb01a