Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(175)

Issue 314790043: Phase 2: ECN implementation for TCP

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 7 months ago by Shravya
Modified:
1 year ago
Reviewers:
Tom Henderson, n.p
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

The patch provided here builds on the Phase 1 work of the ns-3 summer project. ns-3 summer project wiki: https://www.nsnam.org/wiki/ECN_support_for_qdiscs_in_ns-3 Phase 1 code review: https://codereview.appspot.com/306660043/ Any suggestions / feedback would be much appreciated. Regards, Shravya K. S.

Patch Set 1 #

Total comments: 10

Patch Set 2 : Patch set 2 for ecn-tcp #

Patch Set 3 : ecn-tcp patch after SACK inclusion in ns-3-dev #

Patch Set 4 : ecn-tcp patch after minor modifications #

Total comments: 12

Patch Set 5 : Incorporated corrections and added more cases in tcp-ecn-test #

Patch Set 6 : Set 6: Removed iptos packet tag for rst #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1300 lines, -53 lines) Patch
M src/internet/doc/tcp.rst View 1 2 3 4 3 chunks +147 lines, -0 lines 0 comments Download
M src/internet/model/tcp-header.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/internet/model/tcp-socket-base.h View 1 2 3 4 9 chunks +67 lines, -1 line 0 comments Download
M src/internet/model/tcp-socket-base.cc View 1 2 3 4 5 39 chunks +333 lines, -31 lines 0 comments Download
A src/internet/test/tcp-ecn-test.cc View 1 2 3 4 1 chunk +566 lines, -0 lines 0 comments Download
M src/internet/test/tcp-general-test.h View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M src/internet/test/tcp-general-test.cc View 1 2 3 4 1 chunk +17 lines, -0 lines 0 comments Download
M src/internet/wscript View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/traffic-control/doc/red.rst View 1 2 3 4 1 chunk +103 lines, -0 lines 0 comments Download
M src/traffic-control/examples/adaptive-red-tests.cc View 1 2 3 4 9 chunks +37 lines, -13 lines 0 comments Download
M src/traffic-control/examples/red-tests.cc View 1 2 3 4 5 chunks +16 lines, -7 lines 0 comments Download
M src/traffic-control/test/examples-to-run.py View 2 chunks +4 lines, -0 lines 0 comments Download

Messages

Total messages: 13
n.p
Overall the work is well done, there are missing part (they are correctly reported in ...
1 year, 6 months ago (2016-11-04 14:02:37 UTC) #1
Shravya
Hi Natale, Thank you for the feedback. We indeed had a discussion about whether or ...
1 year, 6 months ago (2016-11-05 07:58:56 UTC) #2
n.p
On 2016/11/05 07:58:56, Shravya wrote: > Hi Natale, > > Thank you for the feedback. ...
1 year, 6 months ago (2016-11-14 15:27:15 UTC) #3
Shravya
On 2016/11/14 15:27:15, n.p wrote: > On 2016/11/05 07:58:56, Shravya wrote: > > Hi Natale, ...
1 year, 5 months ago (2016-11-29 08:31:43 UTC) #4
Shravya
Hello Nat, Sorry for the delay. I have addressed the issues you have mentioned. Also, ...
1 year, 4 months ago (2017-01-11 16:13:51 UTC) #5
Shravya
Hello Nat, Sorry for the delay. I have addressed the issues you have mentioned. Also, ...
1 year, 4 months ago (2017-01-11 16:13:53 UTC) #6
Shravya
Hello Nat, Sorry for the delay. I have addressed the issues you have mentioned. Also, ...
1 year, 4 months ago (2017-01-11 16:13:54 UTC) #7
Tom Henderson
I verified that all tests pass with ns-3-dev with this patch, and outside of a ...
1 year, 1 month ago (2017-03-27 18:53:54 UTC) #8
Shravya
Hello Sir, I have incorporated the corrections you have suggested and added few more test ...
1 year, 1 month ago (2017-04-12 11:45:02 UTC) #9
Shravya
Hello Sir, I have incorporated the corrections you have suggested and added few more test ...
1 year, 1 month ago (2017-04-12 11:45:03 UTC) #10
Tom Henderson
On 2017/04/12 11:45:02, Shravya wrote: > Hello Sir, > I have incorporated the corrections you ...
1 year, 1 month ago (2017-04-13 19:57:35 UTC) #11
Shravya
https://codereview.appspot.com/314790043/diff/90001/src/internet/doc/tcp.rst File src/internet/doc/tcp.rst (right): https://codereview.appspot.com/314790043/diff/90001/src/internet/doc/tcp.rst#newcode867 src/internet/doc/tcp.rst:867: 1. Retransmitted packets should not have the CWR bit ...
1 year, 1 month ago (2017-04-14 02:37:27 UTC) #12
Tom Henderson
1 year ago (2017-04-26 03:50:05 UTC) #13
> 
> Sir, the RFC mentions that ECT bits should not be set on pure ACK packets, SYN
,
> SYN-ACK and retransmitted packets. It hasn't mentioned anything regarding
RESET
> packets. I assumed that since RST flag is sent to signal the termination of a
> connection, this packet should not be dropped if there is congestion and
should
> be delivered to the recipient. Please let me know if this is wrong. I will
> remove ECT flags on RST packets.

I can't find anything in the RFCs regarding setting ECT flags in RST packets.  I
doubt that it matters and would probably not include them because the endpoint
is not going to do anything with them.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld e4aa27b