Bug 2070

Summary: Wrong report of Packets and Bytes stored in CoDeL
Product: ns-3 Reporter: natale.patriciello
Component: internetAssignee: George Riley <riley>
Status: RESOLVED FIXED    
Severity: major CC: ns-bugs, tomh, tommaso.pecorella
Priority: P5    
Version: pre-release   
Hardware: PC   
OS: Linux   
Attachments: Patch for CoDeL and Queue model
regression test for CoDeL
Updated comments on the patch

Description natale.patriciello 2015-02-24 10:30:20 UTC 
Created attachment 1974 [details]
Patch for CoDeL and Queue model

Long story: http://mailman.isi.edu/pipermail/ns-developers/2015-February/012581.html
http://mailman.isi.edu/pipermail/ns-developers/2015-February/012582.html

TL;DR 
Each time a packet is dropped, but it was INSIDE the queue, the amounts of bytes and packets should be updated, and the dequeueTrace fired.

Test and patch attached.
Comment 1 natale.patriciello 2015-02-24 10:30:58 UTC 
Created attachment 1975 [details]
regression test for CoDeL
Comment 2 Tommaso Pecorella 2015-02-24 14:57:27 UTC 
+1, push please (subs the comment with a simple "// packet was already in the queue, update stats"
Comment 3 Tom Henderson 2015-02-24 15:11:25 UTC 
I think it is OK to push this, modulo the following:

- I agree with Tommaso's request to modify the comments

- why not retain also the test to check CodelQueue::GetQueueSize() (to ensure these remain consistent)?
Comment 4 natale.patriciello 2015-02-24 16:07:51 UTC 
(In reply to Tom Henderson from comment #3)
> - I agree with Tommaso's request to modify the comments
Agree
> 
> - why not retain also the test to check CodelQueue::GetQueueSize() (to
> ensure these remain consistent)?

I already do that. Each time, in the test code, a check on the size is called:

NS_TEST_EXPECT_MSG_EQ (queue->GetQueueSize (), 0 * modeSize, "There should be no packets in queue");

it is substituted by a call of a function:

QueueTestSize (queue, 0 * modeSize, "There should be no packets in queue");

Which does both the controls, on GetNPackets (or GetNBytes) and GetQueueSize()..

+  void QueueTestSize (Ptr<CoDelQueue> queue, uint32_t size, std::string error)
+  {
+    if (queue->GetMode () == CoDelQueue::QUEUE_MODE_BYTES)
+      {
+        NS_TEST_EXPECT_MSG_EQ (queue->GetNBytes (), size, error);
+      }
+    else if (queue->GetMode () == CoDelQueue::QUEUE_MODE_PACKETS)
+      {
+        NS_TEST_EXPECT_MSG_EQ (queue->GetNPackets (), size, error);
+      }
+
+    NS_TEST_EXPECT_MSG_EQ (queue->GetQueueSize (), size, error);
+  }


Anyway, a little thing I noticed now: I copied/pasted the function all over test classes. Would be better to have a static function (or, only one definition of such function) and checks if I spot all the places where it can be called.
Comment 5 Tom Henderson 2015-02-24 16:34:19 UTC 
(In reply to natale.patriciello from comment #4)
> (In reply to Tom Henderson from comment #3)
> > - I agree with Tommaso's request to modify the comments
> Agree
> > 
> > - why not retain also the test to check CodelQueue::GetQueueSize() (to
> > ensure these remain consistent)?
> 
> I already do that. Each time, in the test code, a check on the size is
> called:


Sorry, I misread that final test macro; I agree with you.


> 
> 
> Anyway, a little thing I noticed now: I copied/pasted the function all over
> test classes. Would be better to have a static function (or, only one
> definition of such function) and checks if I spot all the places where it
> can be called.

I agree on making it one definition.
Comment 6 natale.patriciello 2015-03-01 05:47:10 UTC 
Created attachment 1983 [details]
Updated comments on the patch

Refactored the patch for being included
Comment 7 Tommaso Pecorella 2015-03-01 06:28:11 UTC 
Pushed in changeset:   11227:646733b102d1

Having just one QueueTestSize function is a good idea, but it can't be easily achieved. The NS_TEST macros needs to be in a TestCase class.