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

Issue 314790043: Phase 2: ECN implementation for TCP

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year ago by Shravya
Modified:
5 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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. ...
11 months, 1 week 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, ...
10 months, 4 weeks 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, ...
9 months, 2 weeks 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, ...
9 months, 2 weeks 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, ...
9 months, 2 weeks 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 ...
7 months 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 1 week 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 ...
6 months, 1 week ago (2017-04-14 02:37:27 UTC) #12
Tom Henderson
6 months 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 80a51fa-tainted