Bugzilla – Bug 2070
Wrong report of Packets and Bytes stored in CoDeL
Last modified: 2015-03-01 06:28:11 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.
Created attachment 1975 [details] regression test for CoDeL
+1, push please (subs the comment with a simple "// packet was already in the queue, update stats"
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)?
(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.
(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.
Created attachment 1983 [details] Updated comments on the patch Refactored the patch for being included
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.