9 years, 8 months ago
(2014-08-26 12:44:44 UTC)
#2
Updated patchset
https://codereview.appspot.com/122010043/diff/1/src/internet/model/tcp-highsp...
File src/internet/model/tcp-highspeed.h (right):
https://codereview.appspot.com/122010043/diff/1/src/internet/model/tcp-highsp...
src/internet/model/tcp-highspeed.h:64: float TableLookupB (uint32_t w);
On 2014/08/12 19:20:04, Tommaso Pecorella wrote:
> instead of using two tables, you can simply calculate the value on the fly.
> b(w) will be more precise, while for a(w) there is a small glitch to be aware
> of. In the RFC eq. (1) is inverted with a finite precision.
> As a consequence, the formula for a(w) is slightly wrong for low w values.
> Practically, you can safely use the formulas in appendix B (it's a PERL
> program), BUT with the following caveat:
> a(w) = max(1, floor (a(w)));
> As is, calculate the value, take the floor and then check if it's at least 1.
>
> Note that, by using the formulas, you'll get smoother b(w) values. The RFC
does
> not require you to use the table.
You're right, but those values are from the Linux kernel.. The reason for the
lookup table might be a faster lookup than doing the calculations every time, at
cost of being imprecise.
On 2015/03/24 10:39:18, n.p wrote:
> Updated code wrt the paper submitted to Wns3.
Are there tests, examples, documentation available?
Were there any of the previous in-line comments from previous patchsets that
were not addressed in this update?
- Tom
On 2015/03/24 17:09:23, Tom Henderson wrote:
> On 2015/03/24 10:39:18, n.p wrote:
> > Updated code wrt the paper submitted to Wns3.
>
> Are there tests, examples, documentation available?
For examples, I see that the modifications to tcp-variants-comparison were not
uploaded by upload.py.. I'll add these.
Documentation is in doxygen format.. there are the need to update also the
manual ? I do not see anything related to TCP models (btw, there are some old
references... see for example "The ns-3 object aggregation system is motivated
in strong part by a recognition that a common use case for ns-2 has been the use
of inheritance and polymorphism to extend protocol models. For instance,
specialized versions of TCP such as RenoTcpAgent derive from (and override
functions from) class TcpAgent." in
https://www.nsnam.org/docs/manual/singlehtml/)
It is really difficult to test TCP behavior to date (one of the few test
available is a simple TCP transfer.. nothing specialized for each congestion
control algorithm). To demonstrate the correctness of the implementations, I've
simulated a transfer, and I have analyzed what happen in each situation (slow
start, congestion avoidance, loss, fast retransmit, etc..) by plotting the
behavior of cWnd and ssTh, comparing them with the theoretical values.
> Were there any of the previous in-line comments from previous patchsets that
> were not addressed in this update?
Probably all of Noordwijk. In this state, I'm not sure if it is ready to be
merged (theoretical issues which have arisen during the development. The
original authors are working on a update, from what I know). For the others,
mostly yes. I can detail even more, I see that I missed two thing (enum in
tcpcubic and a missing ns_log_function).
Have a nice day!
On 2015/06/08 22:31:51, Tom Henderson wrote: > comments on BIC variant only. > > Please ...
8 years, 10 months ago
(2015-06-09 16:22:05 UTC)
#8
On 2015/06/08 22:31:51, Tom Henderson wrote:
> comments on BIC variant only.
>
> Please add mailto:ns-3-reviews@googlegroups.com as a cc' on this codereview
issue.
Done
>
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> File src/internet/model/tcp-bic.cc (right):
>
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:29: #define BICTCP_B 4 /*
> why is this a macro? Why not also an attribute? If not attribute, should be
> "const static int BICTCP_B = 4;"
Added as attribute, you are right. Default value (4) as per Linux implementation
(it is 2 in the paper)
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:40: .AddAttribute ("FastConvergence", "Turn
on/off
> fast convergence",
> what publication is this parameter from?
The paper. This boolean value allows to enable/disable fast convergence (default
enabled).
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:52: .AddAttribute ("LowWnd", "Lower bound on
> congestion window (for TCP friendliness)",
> suggest: "threshold window size for engaging BIC response"
done
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:90: NS_LOG_FUNCTION (this);
> in general, when there are arguments to the function, add them to the
> NS_LOG_FUNCTION such as
>
> NS_LOG_FUNCTION (this << threshold);
Done all logging
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:56: .AddAttribute ("SmoothPart",
> "log(B/(B*Smin))/log(B/(B-1))+B, # of RTT from Wmax-B to Wmax",
> what publication is this parameter from?
This isn't in the original publication, but it is in linux source code. More on
that below.
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:227: else if (dist <= 1)
> Why is S_min (= 1) not an attribute, as per the BIC Infocom paper?
This is not S_min. More on that below.
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:270: if (cnt > 20) /* increase cwnd 5% per RTT
*/
> where did this magic number '20' come from? Should it be a defined constant
or
> an attribute? Also, where does the 5% come from?
Reply of the questions above. Some parameters (like smooth_part) and this 5%
increase are not in the paper. I think they cover "real world" issues. For
example, the if (dist <=1 ) case cover when you have, as distance, something < 1
(e.g. m_lastMax=1000, segCwnd = 999) : 1000-999 / [4 as in Linux, in the paper
2]=0.25 would lead to something strange, because without the special clause <=
1, a binary search is done: segCwnd / dist (999/0.25)=3996. Which is a really
big value. So, in this case, Linux approaches the maximum following the
m_smoothPart attribute. Set to 5, it permits to reach the max in 5 RTT. A way to
remove it (to better follow the paper) would be:
if the distance is < 1, go directly the max in this window
which is translated in having m_smoothPart=1.
For the increase on 5% per RTT, it is never mentioned in the paper.
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.cc:336: if (count == 3)
> this value of 3 is an attribute in other TCP variants (e.g. ReTxThreshold",
> "Threshold for fast retransmit" in tcp-tahoe/reno/newreno.cc). Why not do the
> same here?
Done. As always, I hate code duplication, but I'll fix this on GSoC by merging
the Fast retransmit algorithm into TcpSocketBase.
> Also, this class doxygen should state where the model came from since there is
> not a BIC RFC. Is this from the Infocom paper, or some other reference, or a
> combination?
I added the description from a paragraph of my WNS3 paper (which is a good recap
on congestion control, imho).
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> src/internet/model/tcp-bic.h:55: protected:
> why are these not all private? What is the subclassing strategy you are
trying
> to support?
Maybe someone later want to access to these parameters (in case of a BIC
subclass..). For now, they can be all protected/private as you wish. What I
want, in the long run, is to facilitate the writing of subclasses of these
congestion control (e.g. newer version for BIC). With private members, the first
thing one will do is to copy/paste the code (not everyone want to edit things
inside the src/ directory.. because they think it will mess other things
around).
For me, every thing which is not public should be at least protected. What do
you think?
On 2015/06/09 16:22:05, n.p wrote: > > Reply of the questions above. Some parameters (like ...
8 years, 10 months ago
(2015-06-09 17:57:57 UTC)
#9
On 2015/06/09 16:22:05, n.p wrote:
>
> Reply of the questions above. Some parameters (like smooth_part) and this 5%
> increase are not in the paper. I think they cover "real world" issues. For
> example, the if (dist <=1 ) case cover when you have, as distance, something <
1
> (e.g. m_lastMax=1000, segCwnd = 999) : 1000-999 / [4 as in Linux, in the paper
> 2]=0.25 would lead to something strange, because without the special clause <=
> 1, a binary search is done: segCwnd / dist (999/0.25)=3996. Which is a really
> big value. So, in this case, Linux approaches the maximum following the
> m_smoothPart attribute. Set to 5, it permits to reach the max in 5 RTT. A way
to
> remove it (to better follow the paper) would be:
>
> if the distance is < 1, go directly the max in this window
>
> which is translated in having m_smoothPart=1.
>
> For the increase on 5% per RTT, it is never mentioned in the paper.
Please summarize the above explanation somehow and put in as a comment to the
code, for the benefit of future readers.
>
> >
>
https://codereview.appspot.com/122010043/diff/120001/src/internet/model/tcp-b...
> > src/internet/model/tcp-bic.h:55: protected:
> > why are these not all private? What is the subclassing strategy you are
> trying
> > to support?
>
> Maybe someone later want to access to these parameters (in case of a BIC
> subclass..). For now, they can be all protected/private as you wish. What I
> want, in the long run, is to facilitate the writing of subclasses of these
> congestion control (e.g. newer version for BIC). With private members, the
first
> thing one will do is to copy/paste the code (not everyone want to edit things
> inside the src/ directory.. because they think it will mess other things
> around).
> For me, every thing which is not public should be at least protected. What do
> you think?
The normal practice IMO is to make everything that is non-public into private,
and expose things protected as needed (at a later date) or if anticipated for
the subclassing design pattern. Usually the raw access to member variables is
not provided but instead (public or protected) setters and getters if needed.
Opinions probably differ on this design aspect, but I think that in general, we
try to only use protected as needed and not just put everything in protected
'just in case' someone may need it in the future.
It seems to me that there are mostly protected methods and member variables, but
then there are some private non-virtual methods, so I wasn't sure what the
design pattern was intending. Here is what I recommend:
- review each non-public method and think whether you may want to allow future
user to change behavior in a subclass; if so, make it virtual protected.
- make each data member private and if access needed by subclass, implement
SetX/GetX as protected non-virtual (note: any member that is bound to an
attribute is accessible by default)
I've gone through the rest of these files. I would normally say "not ready for ...
8 years, 10 months ago
(2015-06-09 18:42:58 UTC)
#10
I've gone through the rest of these files. I would normally say "not ready for
commit" due to lack of tests, but given we have the GSoC program focused on
this, I will let this point slide.
In general, I recommend:
1) take a pass through all files to fix logging macros as suggested
2) For each new file, please run the check-style.py program on it. Run as
"utils/check-style.py -i -l 3 -f src/internet/model/tcp-hybla.h" (for example)
3) My understanding is that Noordwijk is being held back because it is not
ready, correct?
4) the only major issue to resolve, IMO, is the private/protected data and
methods. I don't want to be pedantic about this issue, leading to tedious
implementation of dozens of setters and getters. My main comment was that it
seemed that a lot of members and data were being made protected, but not all,
and I wondered what drove the decisions, and also whether too much was assumed
about the need to provide everything to future subclasses.
I also realize that some of the issues arise because of the legacy design of
these classes and use of inheritance instead of composition for congestion
control behavior.
So, what I propose that you do is as follows. On a class by class basis, please
review what you have made protected and private (including data members).
Include a general comment in each class that much of the internal implementation
is exposed to facilitate future subclassing. Then, think about what reasonably
needs to be exposed as protected, and what would never need to be exposed (and
should be private)-- if anything -- and make any such changes. I'm not going to
ask you for implementation of lots of setters and getters on data members if you
think that these state variables should be accessible. Maybe you have already
done this type of review of your code, in which case you don't make any changes;
I'm just asking you to revisit in light of my comments, and I'll let you make
the call.
https://codereview.appspot.com/122010043/diff/120001/examples/tcp/tcp-variant...
File examples/tcp/tcp-variants-comparison.cc (right):
https://codereview.appspot.com/122010043/diff/120001/examples/tcp/tcp-variant...
examples/tcp/tcp-variants-comparison.cc:250: { // the default protocol type in
ns3::TcpWestwood is Bic
these comments about the default protocol type (there are four such new ones
proposed in this file) do not apply to protocols other than Westwood. remove
these comments.
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
File src/internet/model/tcp-cubic.cc (right):
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.cc:37: .AddConstructor<TcpCubic> ()
Please add ".SetGroupName ("Internet")" in all of the TypeId methods of your
final patch.
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.cc:104: NS_LOG_FUNCTION_NOARGS ();
use "NS_LOG_FUNCTION (this);"
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.cc:180: NS_LOG_FUNCTION (this);
this << threshold
(I'll stop leaving such comments in the code review-- just do a pass through all
of your files for these).
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
File src/internet/model/tcp-cubic.h (right):
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.h:123: };
(question also applies to BIC) Do you think that Cubic state might want to be
exposed as a traced value, for someone interested in tracking the state of the
cong. control machine? If so, this enum could go public.
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-c...
src/internet/model/tcp-cubic.h:181: void HystartUpdate (const Time& delay);
for API consistency, pass Time in as value (you do so in the next method)
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
File src/internet/model/tcp-highspeed.cc (right):
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
src/internet/model/tcp-highspeed.cc:69: NS_LOG_LOGIC ("TcpHighSpeed receieved
ACK for seq " << seq <<
received
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
File src/internet/model/tcp-highspeed.h (right):
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-h...
src/internet/model/tcp-highspeed.h:77: * \brief Lookup table for a (from RFC
3649)
suggest: 'Lookup table for the coefficient a (from RFC 3649)'
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-n...
File src/internet/model/tcp-noordwijk.cc (right):
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-n...
src/internet/model/tcp-noordwijk.cc:35: // Assumption: defBurstWnd <= m_rWnd ,
so SETUP RIGHT Timer and InitialCwnd
please elaborate this comment
https://codereview.appspot.com/122010043/diff/150051/src/internet/model/tcp-n...
src/internet/model/tcp-noordwijk.cc:176: // " m_bWnd: " <<
m_bWnd);
remove dead code
Issue 122010043: SOCIS2014 - Tcp variants
(Closed)
Created 9 years, 8 months ago by n.p
Modified 8 years, 5 months ago
Reviewers: Tommaso Pecorella, Tom Henderson
Base URL:
Comments: 78
