Issue 306660043: Phase 1: ECN implementation for queue discs

Issue 306660043: Phase 1: ECN implementation for queue discs (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
8 years, 6 months ago by Shravya

Modified:
8 years, 4 months ago

Reviewers:
Mohit Tahiliani, Stefano Avallone, n.p, Tom Henderson

CC:
ns-3-reviews_googlegroups.com

Visibility:
Public.

More Reviews

Description

Explicit Congestion Notification (RFC 3168) is a mechanism for Active Queue Management (AQM) to notify endpoints of congestion that may be developing in a bottleneck queue, without resorting to packet drops. In ns-3, the current queue disciplines (e.g., RED) only support packet drops but not an ECN mode of operation. The patch here provides an implementation of ECN mechanism in RED QueueDisc and tests to verify its functionality. This patch has been developed as a part of the ns-3 summer project listed here: https://www.nsnam.org/wiki/ECN_support_for_qdiscs_in_ns-3 Any suggestions would be much appreciated. Regards, Shravya K. S P.S.: - This patch contains a fix for Bug 2485 - Phase II code is available for review here: https://codereview.appspot.com/314790043/

Patch Set 1 #

Total comments: 15

Patch Set 2 : Revised patch #

Total comments: 25

Patch Set 3 : revised patch #

Created: 8 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+336 lines, -12 lines)			Patch
M	src/internet/doc/ipv4.rst	View		1 chunk	+23 lines, -0 lines	0 comments	Download
M	src/internet/doc/ipv6.rst	View	1	1 chunk	+24 lines, -0 lines	0 comments	Download
M	src/internet/model/ipv4-queue-disc-item.h	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	src/internet/model/ipv4-queue-disc-item.cc	View	1	1 chunk	+13 lines, -0 lines	0 comments	Download
M	src/internet/model/ipv6-header.h	View	1 2	1 chunk	+26 lines, -0 lines	0 comments	Download
M	src/internet/model/ipv6-header.cc	View	1 2	2 chunks	+32 lines, -0 lines	0 comments	Download
M	src/internet/model/ipv6-queue-disc-item.h	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	src/internet/model/ipv6-queue-disc-item.cc	View	1	1 chunk	+12 lines, -0 lines	0 comments	Download
M	src/test/ns3tc/pie-queue-disc-test-suite.cc	View		2 chunks	+7 lines, -0 lines	0 comments	Download
M	src/traffic-control/doc/queue-discs.rst	View		1 chunk	+9 lines, -5 lines	0 comments	Download
M	src/traffic-control/doc/red.rst	View	1 2	2 chunks	+27 lines, -1 line	0 comments	Download
M	src/traffic-control/model/queue-disc.h	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	src/traffic-control/model/red-queue-disc.h	View	1 2	2 chunks	+3 lines, -1 line	0 comments	Download
M	src/traffic-control/model/red-queue-disc.cc	View	1 2	4 chunks	+24 lines, -5 lines	0 comments	Download
M	src/traffic-control/test/codel-queue-disc-test-suite.cc	View		2 chunks	+7 lines, -0 lines	0 comments	Download
M	src/traffic-control/test/red-queue-disc-test-suite.cc	View	1 2	5 chunks	+110 lines, -0 lines	0 comments	Download

Messages

Total messages: 13

Expand All Messages | Collapse All Messages

n.p

Review for the first part. I have some other comments but I prefer to view ...

8 years, 5 months ago (2016-11-04 13:46:45 UTC) #1

Shravya

Hi Natale, Thank you very much for reviewing the code and giving suggestions. I have ...

8 years, 5 months ago (2016-11-05 07:00:49 UTC) #2

Stefano Avallone

Thanks for working on the support of ECN. Please find below my comments. https://codereview.appspot.com/306660043/diff/1/src/internet/doc/ipv6.rst File ...

8 years, 5 months ago (2016-11-06 22:44:58 UTC) #3

Shravya

Thank you for providing detailed reviews. I have attached a revised patch, and provided comments ...

8 years, 5 months ago (2016-11-14 06:00:14 UTC) #4

Thank you for providing detailed reviews. I have attached a revised patch, and
provided comments inline.

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

https://codereview.appspot.com/306660043/diff/1/src/internet/doc/ipv6.rst#new...
src/internet/doc/ipv6.rst:236: 
On 2016/11/06 22:44:58, Stefano Avallone wrote:
> the Traffic class field.

Done.

https://codereview.appspot.com/306660043/diff/1/src/internet/model/ipv4-queue...
File src/internet/model/ipv4-queue-disc-item.cc (right):

https://codereview.appspot.com/306660043/diff/1/src/internet/model/ipv4-queue...
src/internet/model/ipv4-queue-disc-item.cc:86: 
On 2016/11/06 22:44:58, Stefano Avallone wrote:
> It is unlikely that the header has been already added to the packet at this
> point (the header is added after the packet is dequeued from the queue disc).
> However, to be safe, return false if the header has been already added to the
> packet:
> 
> if (!m_headerAdded && (m_header.GetEcn () == Ipv4Header::ECN_ECT1 ||
> m_header.GetEcn () == Ipv4Header::ECN_ECT0))

Done.

https://codereview.appspot.com/306660043/diff/1/src/test/ns3tc/ecn-red-queue-...
File src/test/ns3tc/ecn-red-queue-disc-test-suite.cc (right):

https://codereview.appspot.com/306660043/diff/1/src/test/ns3tc/ecn-red-queue-...
src/test/ns3tc/ecn-red-queue-disc-test-suite.cc:56: class
EcnRedQueueDiscTestCase : public TestCase
On 2016/11/06 22:44:58, Stefano Avallone wrote:
> This test suite is very similar to the existing red queue disc test suite. I
> wonder if it is possible to avoid adding another test suite and instead add a
> couple more tests to the existing red queue disc test suite.

Since ECN tests depend on the internet model, we decided to place them in a
different test suite. Please suggest whether we should still think of retaining
only one test suite.

https://codereview.appspot.com/306660043/diff/1/src/traffic-control/model/red...
File src/traffic-control/model/red-queue-disc.cc (right):

https://codereview.appspot.com/306660043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:352: 
On 2016/11/06 22:44:58, Stefano Avallone wrote:
> Please do not include the patch proposed for bug #2485, which is still under
> discussion.

Done.

https://codereview.appspot.com/306660043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:384: 
On 2016/11/06 22:44:58, Stefano Avallone wrote:
> By doing so, the ECN-enabled version of RED is always used. I would add a
> boolean attribute such as UseECN which determines whether or not to use ECN.
> This check might thus be:
> 
> if (m_useEcn && item->Mark ())
> ...
> 
> Also, I do not think this is the best place to put this. Please have a look at
> my comment in the discussion of bug #2485:
> 
> https://www.nsnam.org/bugzilla/show_bug.cgi?id=2485#c6

Sir,
I have incorporated your suggestion. I have a query with respect to m_useEcn.
Should I declare it in red-queue-disc or queue-disc? Because not all queue-discs
use ECN.

https://codereview.appspot.com/306660043/diff/1/src/traffic-control/model/red...
src/traffic-control/model/red-queue-disc.cc:484: 
On 2016/11/06 22:44:58, Stefano Avallone wrote:
> You should also add m_stats.forcedMark = 0;

Sir,
I had discussed this issue with Tom Sir during implementation. We agreed to
remove forcedMark because although forcedMark was recommended in the original
RED paper but the RFC 3168(section 7) recommends not to mark the packets when
queue length exceeds MaxTh. Besides, the ns-2 version of red also hasn't
implemented forcedMark. 

I can add forcedMark if you still suggest to include it.

Stefano Avallone

Hi Shravya, thanks for updating the patch. I will discuss with Tom to decide what ...

8 years, 5 months ago (2016-11-14 09:42:04 UTC) #5

Mohit Tahiliani

Thanks for the reviews, Natale and Stefano. Shravya, I've added some minor comments; mainly related ...

8 years, 5 months ago (2016-11-14 14:17:51 UTC) #6

Stefano Avallone

Thanks Mohit, I also had noticed all the trailing spaces and the redundant lines in ...

8 years, 5 months ago (2016-11-14 14:29:35 UTC) #7

Mohit Tahiliani

On 2016/11/14 14:29:35, Stefano Avallone wrote: > Thanks Mohit, I also had noticed all the ...

8 years, 5 months ago (2016-11-14 14:32:49 UTC) #8

Tom Henderson

> On 2016/11/06 22:44:58, Stefano Avallone wrote: > > This test suite is very similar ...

8 years, 5 months ago (2016-11-14 15:30:49 UTC) #9

> On 2016/11/06 22:44:58, Stefano Avallone wrote:
> > This test suite is very similar to the existing red queue disc test suite. I
> > wonder if it is possible to avoid adding another test suite and instead add
a
> > couple more tests to the existing red queue disc test suite.
> 
> Since ECN tests depend on the internet model, we decided to place them in a
> different test suite. Please suggest whether we should still think of
retaining
> only one test suite.

I don't agree that these tests must depend on the internet model; instead, the
RedQueueDiscTestItem could support a Mark() implementation that simply marked a
public flag in the object (e.g. bool m_isMarked), and this type of check:

+ NS_TEST_EXPECT_MSG_EQ
((DynamicCast<Ipv4QueueDiscItem>(item)->GetHeader()).GetEcn (),
Ipv4Header::ECN_ECT0, "The packet should be marked");

could be replaced by:

+ NS_TEST_EXPECT_MSG_EQ (item->m_isMarked, true, "The queue disc item should be
marked");

So I agree with Stefano's comment that this may be easier to maintain in the
future if there were one test suite in traffic control module that was extended
to cover ECN markings; e.g. 

RedQueueDiscTestCase::RunRedTest (StringValue mode, bool
ecnEnabledInQueueDiscItems, bool ecnEnabledInQueue)

Whether to make this merge (which involves a bit of work), I defer to Stefano's
preference.

For the second phase of the project, the ECN with TCP tests should probably be
in the src/test/ns3tc directory due to the dependency there.

Regardless of whether these tests are merged or kept as-is, I would like to see
some explanation left in the comments of these tests that describe why the
values observed are judged to be reasonable.  Where it says:

+ NS_TEST_EXPECT_MSG_EQ
((DynamicCast<Ipv4QueueDiscItem>(item)->GetHeader()).GetEcn (),
Ipv4Header::ECN_ECT0, "The packet should be marked");

leave a comment above explaining why with 5 packets, minTh=2, maxTh=5, and 5
packets enqueued, that this last packet was marked.

Similarly, there are later checks such as:

+ NS_TEST_EXPECT_MSG_EQ (st.unforcedMark, 27, "There should be 27 marked packets
with this seed, run number, and stream");

+      NS_TEST_EXPECT_MSG_EQ (st.unforcedMark, 62, "There should be 62 unforced
marked packets  due to probability mark with this seed, run number, and
stream");
+      NS_TEST_EXPECT_MSG_EQ (st.forcedDrop, 51, "There should be 51 forced
drops with this seed, run number, and stream");

Please leave a comment to future maintainers and readers of the code why you
think these values (e.g. 27, 62, 51) are reasonable outcomes based on the
configuration of the queue.

Finally, I suggest that you explicitly set the global run number and seed on
this test also (in addition to AssignStreams).  Otherwise, these tests will fail
if someone decides to re-seed the regression tests in the future; e.g.  

NS_GLOBAL_VALUE="RngRun=3" ./waf --run 'test-runner --suite=ecn-red-queue-disc'

might fail as presently configured.  These statements should be able to fix the
seed and run number.

RngSeedManger::SetSeed (1);
RngSeedManger::SetRun (1);

Stefano Avallone

On 2016/11/14 15:30:49, Tom Henderson wrote: > > I don't agree that these tests must ...

8 years, 5 months ago (2016-11-14 17:14:21 UTC) #10

On 2016/11/14 15:30:49, Tom Henderson wrote:
> 
> I don't agree that these tests must depend on the internet model; instead, the
> RedQueueDiscTestItem could support a Mark() implementation that simply marked
a
> public flag in the object (e.g. bool m_isMarked), and this type of check:
> 
> + NS_TEST_EXPECT_MSG_EQ
> ((DynamicCast<Ipv4QueueDiscItem>(item)->GetHeader()).GetEcn (),
> Ipv4Header::ECN_ECT0, "The packet should be marked");
> 
> could be replaced by:
> 
> + NS_TEST_EXPECT_MSG_EQ (item->m_isMarked, true, "The queue disc item should
be
> marked");
> 
> So I agree with Stefano's comment that this may be easier to maintain in the
> future if there were one test suite in traffic control module that was
extended
> to cover ECN markings; e.g. 
> 
> RedQueueDiscTestCase::RunRedTest (StringValue mode, bool
> ecnEnabledInQueueDiscItems, bool ecnEnabledInQueue)
> 
> Whether to make this merge (which involves a bit of work), I defer to
Stefano's
> preference.

Given that Tom suggested a smart way to avoid depending on the internet model, I
would be in favour of a single test suite. Perhaps the test suite might make use
of two parameters, ecnEnabled and ecnCapablePackets, and perform different
checks on the results based on the values for such parameters. Please note that
the results in case of ecnEnabled=false must be the same as those in case of
ecnEnabled=true and ecnCapablePackets=false. So, you would end up with only two
different types of checks: 1) ecnEnabled=false OR ecnEnabled=true and
ecnCapablePackets=false; 2) ecnEnabled=true and ecnCapablePackets=true.

> For the second phase of the project, the ECN with TCP tests should probably be
> in the src/test/ns3tc directory due to the dependency there.

Totally agree.

> Regardless of whether these tests are merged or kept as-is, I would like to
see
> some explanation left in the comments of these tests that describe why the
> values observed are judged to be reasonable.  Where it says:
> 
> + NS_TEST_EXPECT_MSG_EQ
> ((DynamicCast<Ipv4QueueDiscItem>(item)->GetHeader()).GetEcn (),
> Ipv4Header::ECN_ECT0, "The packet should be marked");
> 
> leave a comment above explaining why with 5 packets, minTh=2, maxTh=5, and 5
> packets enqueued, that this last packet was marked.
> 
> Similarly, there are later checks such as:
> 
> + NS_TEST_EXPECT_MSG_EQ (st.unforcedMark, 27, "There should be 27 marked
packets
> with this seed, run number, and stream");
> 
> 
> +      NS_TEST_EXPECT_MSG_EQ (st.unforcedMark, 62, "There should be 62
unforced
> marked packets  due to probability mark with this seed, run number, and
> stream");
> +      NS_TEST_EXPECT_MSG_EQ (st.forcedDrop, 51, "There should be 51 forced
> drops with this seed, run number, and stream");
> 
> Please leave a comment to future maintainers and readers of the code why you
> think these values (e.g. 27, 62, 51) are reasonable outcomes based on the
> configuration of the queue.

Agree. I do not like magic numbers that much. It would be better if they could
be explained in some way.

> Finally, I suggest that you explicitly set the global run number and seed on
> this test also (in addition to AssignStreams).  Otherwise, these tests will
fail
> if someone decides to re-seed the regression tests in the future; e.g.  
> 
> NS_GLOBAL_VALUE="RngRun=3" ./waf --run 'test-runner
--suite=ecn-red-queue-disc'
> 
> might fail as presently configured.  These statements should be able to fix
the
> seed and run number.
> 
> RngSeedManger::SetSeed (1);
> RngSeedManger::SetRun (1);

Agree.

Shravya

Dear Tom Sir and Stefano Sir, I have uploaded the revised patch with following changes: ...

8 years, 5 months ago (2016-11-17 14:35:34 UTC) #11

Stefano Avallone

Thanks for updating once again the patch. I have yet to review the new version ...

8 years, 5 months ago (2016-11-17 23:33:30 UTC) #12

Stefano Avallone

8 years, 4 months ago (2016-12-13 19:01:22 UTC) #13

This patch has been pushed to ns-3-dev. Could you please close this review?

Thanks,
Stefano

Expand All Messages | Collapse All Messages