suggest to apply after ns-3.26 release

moving to last call

Sorry, I do not agree with this patch. I had a look at the Linux red_enqueue function:

http://lxr.free-electrons.com/source/net/sched/sch_red.c#L59

It looks like that first the average queue length is compared to the thresholds and afterwards the packet is enqueued (and dropped if there is no room for it). I think the rationale is to take every incoming packet into account (if we dropped first the packet, we would not pass it to the red_action function).

So, I think the current ns-3 implementation is fine.

Thanks Stefano for taking a look at this.

I have a following query, assuming the current ns-3 implementation is fine:

- Suppose the incoming packet is marked with ECN after comparing average queue length with thresholds. So the counter for marked packets will be incremented. 

- After this, if queue is found to be full, this marked packet will be dropped, and the counter for dropped packets will also be incremented. 

In my opinion, this may not be correct because both (mark and drop) counters are incremented for the same packet.

Please let me know if I am mistaken.

Thanks again.

I think the situation you describe is fine because for the same packet the unforced/forced mark counter is incremented and the general drop counter (not the unforced/forced drop counter used by RED) is incremented.

By the way, I'm going to leave a few comments on the code review about ECN.

Let me elaborate a bit more (and better). Currently, we have in DoEnqueue:


  if ((GetMode () == Queue::QUEUE_MODE_PACKETS && nQueued >= m_queueLimit) ||
      (GetMode () == Queue::QUEUE_MODE_BYTES && nQueued + item->GetPacketSize() > m_queueLimit))
    {
      NS_LOG_DEBUG ("\t Dropping due to Queue Full " << nQueued);
      dropType = DTYPE_FORCED;
      m_stats.qLimDrop++;
    }

  if (dropType == DTYPE_UNFORCED)
    {
      NS_LOG_DEBUG ("\t Dropping due to Prob Mark " << m_qAvg);
      m_stats.unforcedDrop++;
      Drop (item);
      return false;
    }
  else if (dropType == DTYPE_FORCED)
    {
      NS_LOG_DEBUG ("\t Dropping due to Hard Mark " << m_qAvg);
      m_stats.forcedDrop++;
      Drop (item);
      if (m_isNs1Compat)
        {
          m_count = 0;
          m_countBytes = 0;
        }
      return false;
    }

  bool retval = GetInternalQueue (0)->Enqueue (item);


To resemble what Linux does, it should be something like:


  if (dropType == DTYPE_UNFORCED)
    {
      if (m_useEcn && item->Mark ())
        {
          NS_LOG_DEBUG ("\t Marking due to Prob Mark " << m_qAvg);
          m_stats.unforcedMark++;
        }
      else
        {
          NS_LOG_DEBUG ("\t Dropping due to Prob Mark " << m_qAvg);
          m_stats.unforcedDrop++;
          Drop (item);
          return false;
        }
    }
  else if (dropType == DTYPE_FORCED)
    {
      if (m_useEcn && item->Mark ())
        {
          NS_LOG_DEBUG ("\t Marking due to Hard Mark " << m_qAvg);
          m_stats.forcedMark++;
        }
      else
        {
          NS_LOG_DEBUG ("\t Dropping due to Hard Mark " << m_qAvg);
          m_stats.forcedDrop++;
          Drop (item);
          if (m_isNs1Compat)
            {
              m_count = 0;
              m_countBytes = 0;
            }
          return false;
        }
    }

  bool retval = GetInternalQueue (0)->Enqueue (item);

  if (!retval)
    {
      m_stats.qLimDrop++;
    }


m_useEcn is a boolean attribute to enable/disable using ECN.

Hi Stefano,

I'm sorry for the delay.

Replies are in line:

(In reply to Stefano Avallone from comment #5)
> I think the situation you describe is fine because for the same packet the
> unforced/forced mark counter is incremented and the general drop counter
> (not the unforced/forced drop counter used by RED) is incremented.

with the current code in ns-3-dev, apart the general drop counter, forced drop counter is also incremented. In the code below: 

  if ((GetMode () == Queue::QUEUE_MODE_PACKETS && nQueued >= m_queueLimit) ||
      (GetMode () == Queue::QUEUE_MODE_BYTES && nQueued + item->GetPacketSize() > m_queueLimit))
    {
      NS_LOG_DEBUG ("\t Dropping due to Queue Full " << nQueued);
      dropType = DTYPE_FORCED;
      m_stats.qLimDrop++;
    }

apart from m_stats.qLimDrop++, dropType is set to DTYPE_FORCED, which further increments m_stats.forcedDrop 

[Note: this looks like an another bug to me; both (qLimDrop and forcedDrop) are incremented when queue is full. Incrementing only qLimDrop would be sufficient?]

I've more comments in my next message.

> 
> By the way, I'm going to leave a few comments on the code review about ECN.

Thanks for the review! Really appreciate your support.

(In reply to Stefano Avallone from comment #6)
> Let me elaborate a bit more (and better). Currently, we have in DoEnqueue:
> 
> 
>   if ((GetMode () == Queue::QUEUE_MODE_PACKETS && nQueued >= m_queueLimit) ||
>       (GetMode () == Queue::QUEUE_MODE_BYTES && nQueued +
> item->GetPacketSize() > m_queueLimit))
>     {
>       NS_LOG_DEBUG ("\t Dropping due to Queue Full " << nQueued);
>       dropType = DTYPE_FORCED;
>       m_stats.qLimDrop++;
>     }
> 
>   if (dropType == DTYPE_UNFORCED)
>     {
>       NS_LOG_DEBUG ("\t Dropping due to Prob Mark " << m_qAvg);
>       m_stats.unforcedDrop++;
>       Drop (item);
>       return false;
>     }
>   else if (dropType == DTYPE_FORCED)
>     {
>       NS_LOG_DEBUG ("\t Dropping due to Hard Mark " << m_qAvg);
>       m_stats.forcedDrop++;
>       Drop (item);
>       if (m_isNs1Compat)
>         {
>           m_count = 0;
>           m_countBytes = 0;
>         }
>       return false;
>     }
> 
>   bool retval = GetInternalQueue (0)->Enqueue (item);
> 
> 
> To resemble what Linux does, it should be something like:
> 
> 
>   if (dropType == DTYPE_UNFORCED)
>     {
>       if (m_useEcn && item->Mark ())
>         {
>           NS_LOG_DEBUG ("\t Marking due to Prob Mark " << m_qAvg);
>           m_stats.unforcedMark++;
>         }
>       else
>         {
>           NS_LOG_DEBUG ("\t Dropping due to Prob Mark " << m_qAvg);
>           m_stats.unforcedDrop++;
>           Drop (item);
>           return false;
>         }
>     }
>   else if (dropType == DTYPE_FORCED)
>     {
>       if (m_useEcn && item->Mark ())
>         {
>           NS_LOG_DEBUG ("\t Marking due to Hard Mark " << m_qAvg);
>           m_stats.forcedMark++;
>         }
>       else
>         {
>           NS_LOG_DEBUG ("\t Dropping due to Hard Mark " << m_qAvg);
>           m_stats.forcedDrop++;
>           Drop (item);
>           if (m_isNs1Compat)
>             {
>               m_count = 0;
>               m_countBytes = 0;
>             }
>           return false;
>         }
>     }
> 
>   bool retval = GetInternalQueue (0)->Enqueue (item);
> 
>   if (!retval)
>     {
>       m_stats.qLimDrop++;
>     }

Indeed, this looks better. This modification increments only qLimDrop counter when queue is full, and not the forcedDrop counter.

So as I mentioned in my previous reply, was that a bug?

If yes, then the modifications which you've proposed look fine. Meanwhile, I'll check whether any RED / ARED tests and examples fail due to this modification.

Thank you!

> 
> 
> m_useEcn is a boolean attribute to enable/disable using ECN.

Later today I will open a codereview with the proposed changes to RED (including ECN support). Thanks.

Please have a look at:

https://codereview.appspot.com/314120044/

That patch shows how I intend to add ECN support to ECN (and fix this bug).

Thanks.

(In reply to Stefano Avallone from comment #10)
> Please have a look at:
> 
> https://codereview.appspot.com/314120044/
> 
> That patch shows how I intend to add ECN support to ECN (and fix this bug).
> 
> Thanks.

Hello, what is the verdict on this bug and the two code reviews involved (Stefano and Shravya's)?

Is the path forward to:
1) adopt Stefano's code review
2) have Shravya rebase the rest of the code from her review issue (found here: https://codereview.appspot.com/306660043/) on top of #1
3) commit 1) and then 2) once reviews are done
4) close this bug and the two codereview issues?

If not, please suggest the plan to close out, thanks.

I agree to the path mentioned above.

Thanks.

Hello,

both Mohit and Shravya commented on my code review that they agree with the changes proposed therein. Then, I went ahead and added this stuff to the tc-red branch on my github repo:

https://github.com/stavallo/ns-3-dev-git/commits/tc-red

I pushed three commits there:

1) traffic-control: Add support for marking packets for ECN

This is Shravya's patch except for the changes to the RED queue disc and test

2) traffic-control: (fixes #2485) Add ECN support to RED

This is the patch I published in code review 314120044

3) traffic-control: Add ECN tests to red-queue-disc test suite

This adds ECN tests to the RED test suite. I slightly modified Shravya's patch to get rid of global variables.


The tests introduced with 3) pass. However, after 1) and 2) I get:

List of FAILed tests:
    adaptive-red-queue-disc
    src/traffic-control/examples/adaptive-red-tests --testNumber=1
    src/traffic-control/examples/adaptive-red-tests --testNumber=2

Mohit and Shravya, could you please have a look at these failing tests?

Thanks a lot.

Thanks Stefano.

I modified adaptive-red-queue-disc-test-suite.cc to make it independent of the internet model. This was due since quite some time.

The patch is uploaded here for review: https://codereview.appspot.com/311370043/

I checked this patch on your tc-red github repo, and see that adaptive-red-queue-disc passes now.

Can you take a look at new ARED test suite, and provide your suggestions?

Meanwhile, I'll try to resolve the issue of failing ARED examples.

Thanks again.

Hi Stefano,

Natale also reported that similar examples were failing with the SACK code (See bug 2248), so I have modified these examples accordingly.

The patch is uploaded for review here: https://codereview.appspot.com/311390043/

Along with both the patches (link of first patch is shared in previous comment), I see that all tests pass for the following code:

https://github.com/stavallo/ns-3-dev-git/commits/tc-red

Please let me know your opinion.

Thanks a lot!

Pushed with changeset 12458:3a7ed525f3dc