Issue 292400043: HTCP protocol implementation
|
Created:
8 years ago by Amir.Arc Modified:
7 years, 8 months ago CC:
ns-3-review_googlegroups.com Visibility:
Public. |
Description
Hamilton TCP (H-TCP) implementation in |ns3|
------------------------------------------------
This section describes H-TCP implementation in |ns3|. This implementation is contained in the following files:
.. sourcecode:: text
src/internet/model/tcp-htcp.{cc,h}
Model Description
*****************
H-TCP has been designed for high BDP (Bandwidth-Delay Product) paths. It is a dual mode protocol. In normal condition, it works like traditional TCP with the same rate of increament and decrement for congestion window. However, in high PBD networks, when it finds no congestion on the path after deltal second, it increases the window size based on alpha function in the following:
alpha(delta)=1+10(delta-deltal)+0.5(delta-deltal)^2 (1)
where deltal is a threshold in second for switching between the modes and delta is the elapsed time from last congestion.
During the congestion, it reduces the window size multiply by beta function provided in the reference paper. Calculated throughput between the last two consequative congestions is considered for beta calculation.
References
==========
.. [Leith2004] Douglas Leith, Robert Shorten. 2004. H-TCP: TCP for high-speed and long-distance networks. Proceedings of PFLDnet, Vol. 2004
.. [draft-leith] Douglas Leith, Robert Shorten. 2007. H-TCP: TCP congestion control for high bandwidth-delay product paths. Internet-Draft online
Validation
==========
The H-TCP model is tested using :cpp:class:`HTcpTestSuite` class defined in `src/internet/test/tcp-htcp-test.cc`. The following commands can be used for testing H-TCP:
::
$ ./waf configure --enable-examples --enable-tests
$ ./waf build
$ ./test.py -s tcp-htcp-test
Example
=======
`tcp-variants-comparison.cc` located in ``examples/tcp`` can be used for htcp simulation.
Patch Set 1 #
Total comments: 30
Patch Set 2 : applied review comments #
Total comments: 9
Patch Set 3 : Second revision #
Total comments: 6
Patch Set 4 : third revision #
MessagesTotal messages: 8
Generally the code needs some formatting improvements. There are methods that can be removed (they are a plain re-implementation). Test is not clear, some documentation is needed. Please use tcp.rst for documenting. Keep in touch and tell me when the new version is available, I'd like to include it before the ns-3.26 release. Nat https://codereview.appspot.com/292400043/diff/1/htcp.rst File htcp.rst (right): https://codereview.appspot.com/292400043/diff/1/htcp.rst#newcode9 htcp.rst:9: src/internet/model/tcp-htcp.{cc,h} Put the documentation in the file tcp.rst , under Congestion Control algorithm section https://codereview.appspot.com/292400043/diff/1/htcp.rst#newcode39 htcp.rst:39: $ ./test.py -s tcp-htcp-test Add this validation test under the appropriate section on tcp.rst https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.cc File src/internet/model/tcp-htcp.cc (right): https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:28: if (m_node) { std::clog << Simulator::Now ().GetSeconds () << " [node " << m_node->GetId () << "] "; }*/ Please don't add this, and respect 80 chars in the entire file https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:89: m_delta = Time (0); Please copy parameters from the other socket (sock) https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:107: Useless white lines, please remove them (also in the body of the methods). Leave 1 blank line between each method implementation https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:136: HTcp::UpdateAlpha (Ptr<const TcpSocketState> tcb) tcb isn't used, please remove it from the parameters https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:172: if (abs ((m_throughput - m_lastThroughput) / m_lastThroughput) > 0.2) the first time GetSSthresh is called, m_lastThroughput is 0. Is it the intended behavior? https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:217: check for rtt != 0 https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:243: HTcp::IncreaseWindow (Ptr<TcpSocketState> tcb, uint32_t segmentsAcked) Please don't reimplement methods if you apply no changes. Leave the NewReno one https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.h File src/internet/model/tcp-htcp.h (right): https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.h... src/internet/model/tcp-htcp.h:42: * This class contains the H-TCP implementation of TCP, according to Internet-Draft draft-leith-tcp-htcp-03 and its related paper, 80 chars https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.h... src/internet/model/tcp-htcp.h:104: virtual void CongestionAvoidance (Ptr<TcpSocketState> tcb, uint32_t segmentsAcked); please format everything for 80 chars column limit, and make some "nice" formatting over the member description. Also, put protected member before private ones https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... File src/internet/test/tcp-htcp-test.cc (right): https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.cc:35: static double UpdateAlpha (Time current_time, Time last_congestion, double beta) name of the function is on a new line use (const Time ¤t_time, const Time &last, double beta) as argument list (please note the use of references) naming should be in camelCase https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.cc:50: 80 chars column, empty spaces https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.cc:155: this is not very clear to me; could you please add some description? Even for the decrease. Thanks https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... File src/internet/test/tcp-htcp-test.h (right): https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.h:8: HTcpTest (); This file is useless, please remove it
Sign in to reply to this message.
Hello Nat I updated the files. Please check them and let me know if any other changes are necessary. Best Amir https://codereview.appspot.com/292400043/diff/1/htcp.rst File htcp.rst (right): https://codereview.appspot.com/292400043/diff/1/htcp.rst#newcode9 htcp.rst:9: src/internet/model/tcp-htcp.{cc,h} On 2016/06/04 18:04:39, n.p wrote: > Put the documentation in the file tcp.rst , under Congestion Control algorithm > section Done. https://codereview.appspot.com/292400043/diff/1/htcp.rst#newcode39 htcp.rst:39: $ ./test.py -s tcp-htcp-test On 2016/06/04 18:04:39, n.p wrote: > Add this validation test under the appropriate section on tcp.rst Done. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.cc File src/internet/model/tcp-htcp.cc (right): https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:28: if (m_node) { std::clog << Simulator::Now ().GetSeconds () << " [node " << m_node->GetId () << "] "; }*/ On 2016/06/04 18:04:39, n.p wrote: > Please don't add this, and respect 80 chars in the entire file Done. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:89: m_delta = Time (0); On 2016/06/04 18:04:39, n.p wrote: > Please copy parameters from the other socket (sock) Done. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:107: On 2016/06/04 18:04:39, n.p wrote: > Useless white lines, please remove them (also in the body of the methods). Leave > 1 blank line between each method implementation Done. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:136: HTcp::UpdateAlpha (Ptr<const TcpSocketState> tcb) On 2016/06/04 18:04:39, n.p wrote: > tcb isn't used, please remove it from the parameters Done. It was removed. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:172: if (abs ((m_throughput - m_lastThroughput) / m_lastThroughput) > 0.2) On 2016/06/04 18:04:39, n.p wrote: > the first time GetSSthresh is called, m_lastThroughput is 0. Is it the intended > behavior? m_lastThroughput keeps the throughput of the flow before the congestion. If a flow is initialized, m_lastThroughput is zero; because no congestion has happened yet. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:217: On 2016/06/04 18:04:39, n.p wrote: > check for rtt != 0 if rtt !=0 then it adjusts the values of m_minRtt and m_maxRtt. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.c... src/internet/model/tcp-htcp.cc:243: HTcp::IncreaseWindow (Ptr<TcpSocketState> tcb, uint32_t segmentsAcked) On 2016/06/04 18:04:39, n.p wrote: > Please don't reimplement methods if you apply no changes. Leave the NewReno one It was removed. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.h File src/internet/model/tcp-htcp.h (right): https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.h... src/internet/model/tcp-htcp.h:42: * This class contains the H-TCP implementation of TCP, according to Internet-Draft draft-leith-tcp-htcp-03 and its related paper, On 2016/06/04 18:04:39, n.p wrote: > 80 chars Done. https://codereview.appspot.com/292400043/diff/1/src/internet/model/tcp-htcp.h... src/internet/model/tcp-htcp.h:104: virtual void CongestionAvoidance (Ptr<TcpSocketState> tcb, uint32_t segmentsAcked); On 2016/06/04 18:04:39, n.p wrote: > please format everything for 80 chars column limit, and make some "nice" > formatting over the member description. Also, put protected member before > private ones Done. https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... File src/internet/test/tcp-htcp-test.cc (right): https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.cc:35: static double UpdateAlpha (Time current_time, Time last_congestion, double beta) On 2016/06/04 18:04:40, n.p wrote: > name of the function is on a new line > > use (const Time ¤t_time, const Time &last, double beta) as argument list > (please note the use of references) > > naming should be in camelCase Done. https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.cc:50: On 2016/06/04 18:04:40, n.p wrote: > 80 chars column, empty spaces Done. https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.cc:155: On 2016/06/04 18:04:40, n.p wrote: > this is not very clear to me; could you please add some description? Even for > the decrease. Thanks I added a few line comments for those parts. If it is not enough, please let me know to add more. https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... File src/internet/test/tcp-htcp-test.h (right): https://codereview.appspot.com/292400043/diff/1/src/internet/test/tcp-htcp-te... src/internet/test/tcp-htcp-test.h:8: HTcpTest (); On 2016/06/04 18:04:40, n.p wrote: > This file is useless, please remove it Done.
Sign in to reply to this message.
This looks close to being ready. I'll leave it up to Natale whether to merge for upcoming release or wait for code comments to be resolved (style comments can be fixed by a maintainer). Here are a few high-level comments to match the comments inline: code: 1) copy constructor looks incorrect 2) do some constants in the code need to be changeable by future users, and hence made into attributes? 3) the tests are not checking for correctness of the specific values reached by cWnd-- see my comment about this style: all code needs to have tabs removed and style fixed; run the program 'check-style.py -i -l 3 -f <filename>' on each of them. https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... File src/internet/model/tcp-htcp.cc (right): https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:71: HTcp::HTcp(const HTcp& sock) : Usually the copy constructor takes values from the object that is being copied, rather than initial values. Please check this. https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:138: m_beta = 0.5; if any of these various constants (0.2, 0.5, etc.) should be variable so that users can experiment with changing their values, promote them to attributes. https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... File src/internet/model/tcp-htcp.h (right): https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.h:90: // Internet_Draft June 2007 either move the above comment to the Doxygen or to the implementation https://codereview.appspot.com/292400043/diff/20001/src/internet/test/tcp-htc... File src/internet/test/tcp-htcp-test.cc (right): https://codereview.appspot.com/292400043/diff/20001/src/internet/test/tcp-htc... src/internet/test/tcp-htcp-test.cc:111: * module. This method does the increment test. This testing approach seems to check that the testing methods in this file produce the same output as the implementation methods. It doesn't, however, give much assurance that the methods produce the correct output; what if both methods (which are the same copy-pasted code) are wrong? I'd rather see some checks that cwnd reached a known good explicit value with a brief comment about why you think it is correct to have reached that value. For example, you could pass in an argument to this test for a new parameter called 'uint32_t expectedCwnd' and check that m_state->m_cWnd.Get () equals that value. then below, where you add a test case: + AddTestCase( + new HTcpIncrementTest(38 * 536, 536, 38, ns3::MilliSeconds(1), + ns3::MilliSeconds(900), ns3::MilliSeconds(1000), + "HTcp increment test on cWnd "), TestCase::QUICK); you could add this expected Cwnd value and add a comment such as "cwnd should reach the value X in this test because ..." that will be helpful also to others to understand what is the expected test output and why.
Sign in to reply to this message.
Hello Tom, I am trying to upload the second revision of HTCP, but I have a problem with upload.py. First time, when I got the comments, I downloaded a fresh copy of ns3-dev and added htcp code to that, modified it, did hg commit and used upload.py to upload the code. This time when I got the comments, I used the same repository that I had for the first revision. I did changes and did hg commit. It committed and my repository is current. [admin@localhost ns-3-dev]$ hg tip changeset: 12159:b40bb1b7a50f tag: tip user: Amir Modarresi <amir.arc@gmail.com> date: Sun Jul 31 16:47:54 2016 -0500 summary: revision 2 of the HTcp algorithm but when I try to use upload.py I got the following error. Do you have any suggestion that how I should proceed with the problem? [admin@localhost ns-3-dev]$ python upload.py --oauth2 --issue=292400043 --cc=ns-3-review@googlegroups.com The following files are not added to version control: .cproject .project results.txt scratch/tcp-variants-comparison.cc src/internet/test/tcp-htcp-maintest.cc upload.py Are you sure to continue?(y/N) y *No valid patches found in output from hg diff* Even when I type hg diff, it doesn't show anything; however, hg tip -vp shows all the changes that I have done. Should I download another copy of ns3-dev and add the code there? or there is another solution for that? Thank you Amir On Thu, Jul 28, 2016 at 8:58 AM, <tomh.org@gmail.com> wrote: > This looks close to being ready. I'll leave it up to Natale whether to > merge for upcoming release or wait for code comments to be resolved > (style comments can be fixed by a maintainer). Here are a few > high-level comments to match the comments inline: > > code: > 1) copy constructor looks incorrect > 2) do some constants in the code need to be changeable by future users, > and hence made into attributes? > 3) the tests are not checking for correctness of the specific values > reached by cWnd-- see my comment about this > > style: > all code needs to have tabs removed and style fixed; run the program > 'check-style.py -i -l 3 -f <filename>' on each of them. > > > > > > https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... > File src/internet/model/tcp-htcp.cc (right): > > > https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... > src/internet/model/tcp-htcp.cc:71: HTcp::HTcp(const HTcp& sock) : > Usually the copy constructor takes values from the object that is being > copied, rather than initial values. Please check this. > > > https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... > src/internet/model/tcp-htcp.cc:138: m_beta = 0.5; > if any of these various constants (0.2, 0.5, etc.) should be variable so > that users can experiment with changing their values, promote them to > attributes. > > > https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... > File src/internet/model/tcp-htcp.h (right): > > > https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... > src/internet/model/tcp-htcp.h:90: // Internet_Draft June 2007 > either move the above comment to the Doxygen or to the implementation > > > https://codereview.appspot.com/292400043/diff/20001/src/internet/test/tcp-htc... > File src/internet/test/tcp-htcp-test.cc (right): > > > https://codereview.appspot.com/292400043/diff/20001/src/internet/test/tcp-htc... > src/internet/test/tcp-htcp-test.cc:111: * module. This method does the > increment test. > This testing approach seems to check that the testing methods in this > file produce the same output as the implementation methods. It doesn't, > however, give much assurance that the methods produce the correct > output; what if both methods (which are the same copy-pasted code) are > wrong? > > I'd rather see some checks that cwnd reached a known good explicit value > with a brief comment about why you think it is correct to have reached > that value. For example, you could pass in an argument to this test for > a new parameter called 'uint32_t expectedCwnd' and check that > m_state->m_cWnd.Get () equals that value. > > then below, where you add a test case: > > + AddTestCase( > + new HTcpIncrementTest(38 * 536, 536, 38, > ns3::MilliSeconds(1), > + ns3::MilliSeconds(900), > ns3::MilliSeconds(1000), > + "HTcp increment test on > cWnd "), TestCase::QUICK); > > you could add this expected Cwnd value and add a comment such as "cwnd > should reach the value X in this test because ..." > > that will be helpful also to others to understand what is the expected > test output and why. > > https://codereview.appspot.com/292400043/ >
Sign in to reply to this message.
On 08/01/2016 05:19 AM, Amir Modarresi wrote: > Hello Tom, > > I am trying to upload the second revision of HTCP, but I have a > problem with upload.py. > > First time, when I got the comments, I downloaded a fresh copy of > ns3-dev and added htcp code to that, modified it, did hg commit and > used upload.py to upload the code. > This time when I got the comments, I used the same repository that I > had for the first revision. I did changes and did hg commit. It > committed and my repository is current. > > [admin@localhost ns-3-dev]$ hg tip > changeset: 12159:b40bb1b7a50f > tag: tip > user: Amir Modarresi <amir.arc@gmail.com > <mailto:amir.arc@gmail.com>> > date: Sun Jul 31 16:47:54 2016 -0500 > summary: revision 2 of the HTcp algorithm > > but when I try to use upload.py I got the following error. Do you have > any suggestion that how I should proceed with the problem? > > [admin@localhost ns-3-dev]$ python upload.py --oauth2 > --issue=292400043 --cc=ns-3-review@googlegroups.com > <mailto:ns-3-review@googlegroups.com> > The following files are not added to version control: > .cproject > .project > results.txt > scratch/tcp-variants-comparison.cc > src/internet/test/tcp-htcp-maintest.cc > upload.py > Are you sure to continue?(y/N) y > *No valid patches found in output from hg diff* > > Even when I type hg diff, it doesn't show anything; however, hg tip > -vp shows all the changes that I have done. I see a couple of problems in the above: 1) src/internet/test/tcp-htcp-maintest.cc is not included in the repository; did you forget to add it? (also, is there an extension of tcp-variants-comparison.cc that should be committed-- I see that you have it in scratch/) 2) When this error occurs ' *No valid patches found in output from hg diff* ' it usually means that you forgot to commit your update. However, in this case, you say that you have done that. It may instead be complaining since you are not specifying a previous revision number (rev). See this wiki page: https://www.nsnam.org/wiki/HOWTO_control_Rietveld_patch_generation#Updating_a... So, let's assume that your original patch was rev 12158, and was applied against the base revision 12157. For your second upload, you should again specify the base as 12157 by passing also the argument '--rev=12157' in the command above. > > Should I download another copy of ns3-dev and add the code there? or > there is another solution for that? I suggest to try to resolve it on the repo that you previously started. Thank you for updating the issue. If it still doesn't work for you, please just send Natale and me your diff directly (the complete diff against ns-3-dev code; i.e. patchsets 1 and 2 combined). - Tom
Sign in to reply to this message.
Hello, Please consider this patch for review. Thank you Amir https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... File src/internet/model/tcp-htcp.cc (right): https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:71: HTcp::HTcp(const HTcp& sock) : On 2016/07/28 12:58:45, Tom Henderson wrote: > Usually the copy constructor takes values from the object that is being copied, > rather than initial values. Please check this. Done. https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:138: m_beta = 0.5; On 2016/07/28 12:58:45, Tom Henderson wrote: > if any of these various constants (0.2, 0.5, etc.) should be variable so that > users can experiment with changing their values, promote them to attributes. Done. https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... File src/internet/model/tcp-htcp.h (right): https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.h:90: // Internet_Draft June 2007 On 2016/07/28 12:58:45, Tom Henderson wrote: > either move the above comment to the Doxygen or to the implementation Moved to implementation https://codereview.appspot.com/292400043/diff/20001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.h:90: // Internet_Draft June 2007 On 2016/07/28 12:58:45, Tom Henderson wrote: > either move the above comment to the Doxygen or to the implementation Done. https://codereview.appspot.com/292400043/diff/20001/src/internet/test/tcp-htc... File src/internet/test/tcp-htcp-test.cc (right): https://codereview.appspot.com/292400043/diff/20001/src/internet/test/tcp-htc... src/internet/test/tcp-htcp-test.cc:111: * module. This method does the increment test. On 2016/07/28 12:58:45, Tom Henderson wrote: > This testing approach seems to check that the testing methods in this file > produce the same output as the implementation methods. It doesn't, however, > give much assurance that the methods produce the correct output; what if both > methods (which are the same copy-pasted code) are wrong? > > I'd rather see some checks that cwnd reached a known good explicit value with a > brief comment about why you think it is correct to have reached that value. For > example, you could pass in an argument to this test for a new parameter called > 'uint32_t expectedCwnd' and check that m_state->m_cWnd.Get () equals that value. > > then below, where you add a test case: > > + AddTestCase( > + new HTcpIncrementTest(38 * 536, 536, 38, ns3::MilliSeconds(1), > + ns3::MilliSeconds(900), ns3::MilliSeconds(1000), > + "HTcp increment test on cWnd "), TestCase::QUICK); > > you could add this expected Cwnd value and add a comment such as "cwnd should > reach the value X in this test because ..." > > that will be helpful also to others to understand what is the expected test > output and why. > I added expectedCwnd which stores the results calculated from the algorithm by hand. Therefore, another argument was added to the AddTestCase method.
Sign in to reply to this message.
There are a few more things that should be handled either before or just after a merge: 1) tcp-variants-comparison.cc needs patched to support this, and also, it would be nice to list, in the H-TCP documentation, the set of command line arguments to this example that exemplifies a high BDP network that H-TCP is designed to perform well in (so users can contrast it against NewReno or other variants in such high BDP network). 2) the special values used for expected cwnd in the test suite should be documented where they came from 3) possible bug in the update alpha code (0.25 vs. 0.5)? https://codereview.appspot.com/292400043/diff/40001/src/internet/model/tcp-ht... File src/internet/model/tcp-htcp.cc (right): https://codereview.appspot.com/292400043/diff/40001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:62: m_throughputRatio (0.2), what I meant was not to convert them to member variables but to attributes. But I read through the Internet Draft and it doesn't seem to me that these are likely to be changed by users. https://codereview.appspot.com/292400043/diff/40001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:139: m_alpha = (1 + 10 * diffSec + 0.25 * (diffSec * diffSec)); //in seconds why '0.25' instead of '0.5'? The Internet-Draft says it is 0.5(Delta-Delta_L)^2, but I don't think that it means that 0.5 is also squared. https://codereview.appspot.com/292400043/diff/40001/src/internet/test/tcp-htc... File src/internet/test/tcp-htcp-test.cc (right): https://codereview.appspot.com/292400043/diff/40001/src/internet/test/tcp-htc... src/internet/test/tcp-htcp-test.cc:132: 20383,"HTcp increment test on cWnd "), TestCase::QUICK); I think that these expected cWnd values need documenting (20383, 40, 76671). Are they sensitive in any way to changes that are made elsewhere in ns-3, or can we assume that 20383 will always work?
Sign in to reply to this message.
Dear All, I posted the HTcp code for review. I appreciate it, if you may review the code and give me your comments and suggestion. Thank you Amir https://codereview.appspot.com/292400043/diff/40001/src/internet/model/tcp-ht... File src/internet/model/tcp-htcp.cc (right): https://codereview.appspot.com/292400043/diff/40001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:62: m_throughputRatio (0.2), On 2016/08/02 05:36:54, Tom Henderson wrote: > what I meant was not to convert them to member variables but to attributes. But > I read through the Internet Draft and it doesn't seem to me that these are > likely to be changed by users. These are the best values based on the authors' experiments; however, they can be changed for other experiments. If you want me to change them, please let me know; although I haven't got your mean by "attribute". Do you mean using [[..]] construct or defining a constant variable in the module? https://codereview.appspot.com/292400043/diff/40001/src/internet/model/tcp-ht... src/internet/model/tcp-htcp.cc:139: m_alpha = (1 + 10 * diffSec + 0.25 * (diffSec * diffSec)); //in seconds On 2016/08/02 05:36:54, Tom Henderson wrote: > why '0.25' instead of '0.5'? The Internet-Draft says it is > 0.5(Delta-Delta_L)^2, but I don't think that it means that 0.5 is also squared. Yes, you are right. In the internet draft, it has been written in a confusing format, but in their papers, it is (1/2)^2. Ref: http://www.hamilton.ie/net/htcp2005.pdf http://www.hamilton.ie/net/htcp3.pdf https://codereview.appspot.com/292400043/diff/40001/src/internet/test/tcp-htc... File src/internet/test/tcp-htcp-test.cc (right): https://codereview.appspot.com/292400043/diff/40001/src/internet/test/tcp-htc... src/internet/test/tcp-htcp-test.cc:132: 20383,"HTcp increment test on cWnd "), TestCase::QUICK); On 2016/08/02 05:36:54, Tom Henderson wrote: > I think that these expected cWnd values need documenting (20383, 40, 76671). > Are they sensitive in any way to changes that are made elsewhere in ns-3, or can > we assume that 20383 will always work? I added more comments to the test module. They are not special values and they don't affect ns-3 in any ways. I chose them randomly and calculated their associated cWnd by hand as m_expectedCwnd. If you want me to change the test totally, please let me know.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
