Phase 2: ECN implementation for TCP

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.

[n.p, 2016-11-04 14:02:37]

Overall the work is well done, there are missing part (they are correctly
reported in the documentation) but it is a good start.

The only thing I can say is that is missing a common part for the queue to
signal how many packets have been marked through a traced value.

Another question is if other queue disc rather than RedQueue can be subclassed
to add the possibility to mark a packet. How this can be done? A section in the
documentation with an example would be wonderful.

Thank you for the work; looking forward for a fastest merge.

https://codereview.appspot.com/314790043/diff/1/src/internet/doc/tcp.rst
File src/internet/doc/tcp.rst (right):

https://codereview.appspot.com/314790043/diff/1/src/internet/doc/tcp.rst#newc...
src/internet/doc/tcp.rst:688: 
to mantain the naming compability, I would use "ECN_DISABLED"

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.cc (left):

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:399: 
the initialization phase misses one member variable (in each constructors)

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:1191: 
If the flag is not valid, is it simply ignored?

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:1217: 
Again, if the received flag is not valid, is it ignored?

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:2674: 
typo: retransmitted

[Shravya, 2016-11-05 07:58:56]

Hi Natale,

Thank you for the feedback.

We indeed had a discussion about whether or not to include trace source for the
marked packets. As of now, we omitted it because not all queue discs use ECN
marking, e.g., PfifoFastQueueDisc. We decided to add it later, if required.
Currently, the stats of marked packets can be obtained from m_stats.unforcedMark
in RedQueueDisc. I will add this in the documentation for the reader to know how
to obtain stats of marked packets. 

About adding ECN support to other queue discs - I've extended CoDel and PIE also
to support ECN, and plan to send them for review once this code is accepted and
merged. I'll check if I can generalize the procedure to add ECN support in queue
discs and put it in the doc.

I'll soon upload the new patch with revisions.

Thanks again, for taking out time to review the code.

https://codereview.appspot.com/314790043/diff/1/src/internet/doc/tcp.rst
File src/internet/doc/tcp.rst (right):

https://codereview.appspot.com/314790043/diff/1/src/internet/doc/tcp.rst#newc...
src/internet/doc/tcp.rst:688: 
On 2016/11/04 14:02:37, n.p wrote:
> to mantain the naming compability, I would use "ECN_DISABLED"

Yes, ECN_DISABLED seems to be a better option here because these are internal
states of ECN, unlike the previous one which is a ECN codepoint.

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.cc (left):

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:399: 
On 2016/11/04 14:02:37, n.p wrote:
> the initialization phase misses one member variable (in each constructors)

Done.

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:1191: 
On 2016/11/04 14:02:37, n.p wrote:
> If the flag is not valid, is it simply ignored?

Yes as per RFC 3168, section 6.1.5

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:1217: 
On 2016/11/04 14:02:37, n.p wrote:
> Again, if the received flag is not valid, is it ignored?

Yes, for the same reason as mentioned before

https://codereview.appspot.com/314790043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:2674: 
On 2016/11/04 14:02:37, n.p wrote:
> typo: retransmitted

Done.

[n.p, 2016-11-14 15:27:15]

On 2016/11/05 07:58:56, Shravya wrote:
> Hi Natale,
> 
> Thank you for the feedback.
> 
> We indeed had a discussion about whether or not to include trace source for
the
> marked packets. As of now, we omitted it because not all queue discs use ECN
> marking, e.g., PfifoFastQueueDisc. We decided to add it later, if required.
> Currently, the stats of marked packets can be obtained from
m_stats.unforcedMark
> in RedQueueDisc. I will add this in the documentation for the reader to know
how
> to obtain stats of marked packets. 
> 
> About adding ECN support to other queue discs - I've extended CoDel and PIE
also
> to support ECN, and plan to send them for review once this code is accepted
and
> merged. I'll check if I can generalize the procedure to add ECN support in
queue
> discs and put it in the doc.
> 
> I'll soon upload the new patch with revisions.


I have another request; it seems is missing a test case for the L4 (TCP) for
checking the ECN option (set/get and if segments effectively goes out with the
ECN bit, and if the sender/receiver honours the setting).

[Shravya, 2016-11-29 08:31:43]

On 2016/11/14 15:27:15, n.p wrote:
> On 2016/11/05 07:58:56, Shravya wrote:
> > Hi Natale,
> > 
> > Thank you for the feedback.
> > 
> > We indeed had a discussion about whether or not to include trace source for
> the
> > marked packets. As of now, we omitted it because not all queue discs use ECN
> > marking, e.g., PfifoFastQueueDisc. We decided to add it later, if required.
> > Currently, the stats of marked packets can be obtained from
> m_stats.unforcedMark
> > in RedQueueDisc. I will add this in the documentation for the reader to know
> how
> > to obtain stats of marked packets. 
> > 
> > About adding ECN support to other queue discs - I've extended CoDel and PIE
> also
> > to support ECN, and plan to send them for review once this code is accepted
> and
> > merged. I'll check if I can generalize the procedure to add ECN support in
> queue
> > discs and put it in the doc.
> > 
> > I'll soon upload the new patch with revisions.
> 
> 
> I have another request; it seems is missing a test case for the L4 (TCP) for
> checking the ECN option (set/get and if segments effectively goes out with the
> ECN bit, and if the sender/receiver honours the setting).

Hi Natale,

Thanks for your feedback, and very sorry for the late response. 

Phase 1 code is undergoing some changes, and once that is finalised and merged,
I shall again start working on Phase 2 and implement test-suite as you have
mentioned.

Thanks again for the reviews.

Regards,
Shravya

[Shravya, 2017-01-11 16:13:51]

Hello Nat,
Sorry for the delay. I have addressed the issues you have mentioned. Also, I
have added the test for the L4 (TCP) for checking the ECN option (set/get and if
segments effectively goes out with the ECN bit, and if the sender/receiver
honours the setting).

[Shravya, 2017-01-11 16:13:53]

Hello Nat,
Sorry for the delay. I have addressed the issues you have mentioned. Also, I
have added the test for the L4 (TCP) for checking the ECN option (set/get and if
segments effectively goes out with the ECN bit, and if the sender/receiver
honours the setting).

[Shravya, 2017-01-11 16:13:54]

Hello Nat,
Sorry for the delay. I have addressed the issues you have mentioned. Also, I
have added the test for the L4 (TCP) for checking the ECN option (set/get and if
segments effectively goes out with the ECN bit, and if the sender/receiver
honours the setting).

[Tom Henderson, 2017-03-27 18:53:54]

I verified that all tests pass with ns-3-dev with this patch, and outside of a
couple of small naming nits, the code looks good and possibly ready to merge.

I have a general comment about how to document that this is tested and verified.
 I noticed that your documentation patch to tcp.rst does not patch the
'Validation' section.

If, for example, you run ./waf --run 'adaptive-red-tests --testNumber=3' you
will get this output:

*** ARED stats from Node 2 queue ***
	 1 drops due to prob mark
	 1 marks due to prob mark
	 0 drops due to hard mark
	 187 drops due to queue full

How do I know what to make of this; is the test behaving correctly?

Users and future maintainers may not know what ns-2 test #3 actually does.  Can
you instead document what each numbered (ECN-related) test accomplishes, and
what is the expected output, in the Validation section

For instance, something like this:

"./waf --run 'adaptive-red-tests --testNumber=3' tests that ECN works as follows
(describe...).  As a result, the test should output this:

*** ARED stats from Node 2 queue ***
	 1 drops due to prob mark
	 1 marks due to prob mark
	 0 drops due to hard mark
	 187 drops due to queue full

These counts are expected because (explain why...)"

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#...
src/internet/doc/tcp.rst:867: 1. Retransmitted packets should not have the CWR
bit set (Section 6.1.5).
Why is this not yet supported?  The code appears to set ECT bits off during
retransmissions.

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:166: .AddTraceSource ("ECNState",
ns-3 naming convention would be to use "EcnState"  (avoid all capitals)

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:1474: p->AddPacketTag (ipTclassTag);
How does setting this tag interact with the setting of this tag for ToS/DSCP
reasons?  Is it logically OR'ed with DSCP bits somewhere else?

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-ecn...
File src/internet/test/tcp-ecn-test.cc (right):

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-ecn...
src/internet/test/tcp-ecn-test.cc:21: 
Please add documentation about what this is testing; e.g.

This test suite will run four combinations of enabling ECN (sender off, receiver
off; sender on, receiver off; etc.)
and check that the TOS byte of eventual packets transmitted or received have
ECT, CWR, and ECE set correctly (or not).

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-gen...
File src/internet/test/tcp-general-test.h (right):

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-gen...
src/internet/test/tcp-general-test.h:578: void SetECN (SocketWho who);
ns-3 coding style would be 'SetEcn'

[Shravya, 2017-04-12 11:45:02]

Hello Sir,
I have incorporated the corrections you have suggested and added few more test
cases in tcp-ecn-test.cc. Please let me know if there are any corrections.

Regards,
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#...
src/internet/doc/tcp.rst:867: 1. Retransmitted packets should not have the CWR
bit set (Section 6.1.5).
On 2017/03/27 18:53:54, Tom Henderson wrote:
> Why is this not yet supported?  The code appears to set ECT bits off during
> retransmissions.

Done.

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:166: .AddTraceSource ("ECNState",
On 2017/03/27 18:53:54, Tom Henderson wrote:
> ns-3 naming convention would be to use "EcnState"  (avoid all capitals)

Done.

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:1474: p->AddPacketTag (ipTclassTag);
On 2017/03/27 18:53:54, Tom Henderson wrote:
> How does setting this tag interact with the setting of this tag for ToS/DSCP
> reasons?  Is it logically OR'ed with DSCP bits somewhere else?

Sir, there was no tag for ToS here. This is why I just added the tag for ect.

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-ecn...
File src/internet/test/tcp-ecn-test.cc (right):

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-ecn...
src/internet/test/tcp-ecn-test.cc:21: 
On 2017/03/27 18:53:54, Tom Henderson wrote:
> Please add documentation about what this is testing; e.g.
> 
> This test suite will run four combinations of enabling ECN (sender off,
receiver
> off; sender on, receiver off; etc.)
> and check that the TOS byte of eventual packets transmitted or received have
> ECT, CWR, and ECE set correctly (or not).

Done.

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-gen...
File src/internet/test/tcp-general-test.h (right):

https://codereview.appspot.com/314790043/diff/90001/src/internet/test/tcp-gen...
src/internet/test/tcp-general-test.h:578: void SetECN (SocketWho who);
On 2017/03/27 18:53:54, Tom Henderson wrote:
> ns-3 coding style would be 'SetEcn'

Done.

[Shravya, 2017-04-12 11:45:03]

Hello Sir,
I have incorporated the corrections you have suggested and added few more test
cases in tcp-ecn-test.cc. Please let me know if there are any corrections.

Regards,
Shravya

[Tom Henderson, 2017-04-13 19:57:35]

On 2017/04/12 11:45:02, Shravya wrote:
> Hello Sir,
> I have incorporated the corrections you have suggested and added few more test
> cases in tcp-ecn-test.cc. Please let me know if there are any corrections.
> 
> Regards,
> 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#...
> src/internet/doc/tcp.rst:867: 1. Retransmitted packets should not have the CWR
> bit set (Section 6.1.5).
> On 2017/03/27 18:53:54, Tom Henderson wrote:
> > Why is this not yet supported?  The code appears to set ECT bits off during
> > retransmissions.
> 
> Done.

Can you clarify what you mean by 'Done'?  Is the open issue in the documentation
still valid, or does the documentation need to change?

> 
>
https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
> src/internet/model/tcp-socket-base.cc:1474: p->AddPacketTag (ipTclassTag);
> On 2017/03/27 18:53:54, Tom Henderson wrote:
> > How does setting this tag interact with the setting of this tag for ToS/DSCP
> > reasons?  Is it logically OR'ed with DSCP bits somewhere else?
> 
> Sir, there was no tag for ToS here. This is why I just added the tag for ect.

OK, so RST packets should be set ECT(0)?  Does it matter?

[Shravya, 2017-04-14 02:37:27]

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#...
src/internet/doc/tcp.rst:867: 1. Retransmitted packets should not have the CWR
bit set (Section 6.1.5).
On 2017/04/12 11:45:01, Shravya wrote:
> On 2017/03/27 18:53:54, Tom Henderson wrote:
> > Why is this not yet supported?  The code appears to set ECT bits off during
> > retransmissions.
> 
> Done.

Yes sir, I modified the code to set CWR bits off during re-transmissions. I had
missed this part initially and I have corrected it in the latest patch.

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/314790043/diff/90001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:1474: p->AddPacketTag (ipTclassTag);
On 2017/04/12 11:45:01, Shravya wrote:
> On 2017/03/27 18:53:54, Tom Henderson wrote:
> > How does setting this tag interact with the setting of this tag for ToS/DSCP
> > reasons?  Is it logically OR'ed with DSCP bits somewhere else?
> 
> Sir, there was no tag for ToS here. This is why I just added the tag for ect.

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.

[Tom Henderson, 2017-04-26 03:50:05]

> 
> 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.