Re: [aqm] I-D Action: draft-ietf-aqm-codel-03.txt

[Polina Goltsman <[e-mail]>]

Dear Wesley, Dear All,

First of all our feedback regarding different "re-entering dropping 
state" in the document and in the Linux implementation 
(http://www.ietf.org/mail-archive/web/aqm/current/msg01686.html) was not 
addressed.

As FQ-CoDel  relies on CoDel, this issue is also (partly) relevant for 
the FQ-CoDel document. In the introduction FQ-CoDel references ns-3 and 
Linux implementations where the first one uses the re-entering logic 
from the CoDel document while the second from CoDel Linux 
implementation. The algorithm that has seen widespread testing according 
to Section 7 is (I suppose) the Linux version. Is this situation 
acceptable for an algorithm specification?

/[since this comment was supposed to be sent before the end of 2015, 
feel free to (silently) ignore it]/
Second, unlike Rasool Al-Saadi (see 
http://www.ietf.org/mail-archive/web/aqm/current/msg01693.html) I do not 
like the document. Although I agree that the pseudocode is sufficient to 
create a working implementation, however, in my opinion, the rest of the 
document makes implementing CoDeL more confusing (at least without 
reading [CODEL2012] first). Is it normal for a RFC, which, as I assume, 
should primarily contain an algorithm specification to contain the 
algorithm specification ONLY in form of pseudocode?
These are two items that I found the most confusing:
(1) section 3 introduces estimator, setpoint, and control loop which are 
not clearly distinguishable in pseudocode. It would be nice if section 4 
explained how the three entities transition into the routines in 
pseudocode.
(2) IMHO, there is some missing explanations. For example, the document 
never says how exactly "bad"/persistent queue is determined. The 
document says in Section 3.1 that the minimum is tracked, but it never 
says that persistent queue is when the minimum is above setpoint/target 
for at least an interval. As another example, Section 3.3 could say how 
the controller looks like exactly.

I also have some small comments and nits regarding the latest version (-03):

---

in the whole document there are several places of using "we" to 
(supposedly) refer to the authors. According to Bob Briscoe's message 
(http://www.ietf.org/mail-archive/web/aqm/current/msg01805.html), "we" 
do not necessary refer to the authors in a rfc. Are these "we"s ok?

---

abstract:

This document describes a general*framework*  called CoDel

IMHO the word "framework" is too overloaded and too broad. It might be 
better to call CoDel an AQM or something that has "queue" in it. Other 
ways (e.g., http://queue.acm.org/detail.cfm?id=2839461) probably also 
use it to control a queue.

---

section 1:

(nit)
    Despite this
    awareness, the problem has only gotten worse as Moore's Law growth in
    memory density fueled an exponential increase in buffer pool size.
    Efforts to deploy AQM have been frustrated by difficult configuration
    and negative impact on network utilization. *This problem,* recently
    christened "bufferbloat", [TSV2011] [BB2011] has become increasingly
    important throughout the Internet but particularly at the consumer
    edge.

it reads like "this problem" refers to "efforts to deploy AQM" and not 
to "increase in buffer pool size"

---

(nit)

    To understand
    queue management, it is critical to understand the difference between
    the necessary, useful*"good" queue*, and the counterproductive*"bad" queue*.

strictly speaking "good" and "bad" queue were not defined either before 
or in this sentence (it is explained in section 3 or in [CODEL2012])

---

(nit)

    o  treat "good queue" and "bad queue" differently, that is, keep
       delay low while permitting necessary bursts of traffic

(1) see above
(2) I would say that the goal is "to keep delay low while ..." and to 
treat good/bad queue differently is a mechanism used to achieve the goal

---

section 2:

(nit)
       In this document, *the characters ">>" preceding an indented line(s)*
    indicates a compliance requirement statement using the key words
    listed above.  This convention aids reviewers in quickly identifying
    or finding the explicit compliance requirements of this RFC.

I maybe missing something, but I can't find any use of "characters ">>"" 
anywhere except this line in any (txt,html, pdf) of the versions.

---

section 3:

generally IMHO it would be nice to introduce /interval/ and /target/ so 
it is more obvious that these are parameters (or some other kind of 
variables). Interval is more or less obvious (although imho should be 
more explicit). The target is first introduced at the end of section 3.2 as:

    This results in a particularly simple form for the
    setpoint: the ideal range for the permitted standing queue is between
    5% and 10% of the TCP connection's RTT.  Thus*target*  is simply 5% of
    the interval ofsection 3.1 
<https://tools.ietf.org/html/draft-ietf-aqm-codel-03#section-3.1>.

It was not previously defined that /target/ is a parameter with a 
meaning of /setpoint/. I don't think that this is clear for people who 
do not know what target is in advance.

[It seems that it was intended to introduce /interval/ and /target/ 
parameters in section 4]

---

    Instead of averages
    *we recommend*  tracking the minimum sojourn time, then,

"we recommend" or "CoDeL does"? According to the last paragraph in 
Section 3.1, tracking minimum is a core part of CoDeL:

    These two innovations, use of sojourn time as observed values*and the local minimum as the statistic to monitor queue congestion*  are key to
    CoDel's estimator building block.

---

    Since the peak queue delay*is simply f r*, power is solely a function

is it f/r ?

---
(nit)

    The only remaining building block needed for a basic AQM is a
    'control loop' algorithm to effectively drive the queueing system
    from any*'persistent queue above target'*  state to a state where the
    *persistent queue is below target*.

It would be nice to explicitly state that the "persistent queue above 
target" is indication of "bad" queue/congestion somewhere above or in 
this line. I think that these terms are used in [codel2012] but were not 
used before in the document. See also the next item.

---
In section 3.3 there are three "states":

  *

    'persistent queue above target' state

  *

    'has persistent queue' state

  *

      dropping state

It would be nice to a) use the same one b) explain how CoDeL gets in/out 
of there

---

(probably nit)

    When
    *the minimum sojourn time first crosses the target*  and*CoDel drops a packet*, the earliest the controller could see the effect of the drop

from current sentence it could be inferred that the two events occur at 
the same time which is not the case. Maybe " when persistent queue above 
target is detected" would be better.

---

    variation is less than the target, and so the*initial drop spacing SHOULD be set to the estimator's interval plus 
twice the target (i.e., initial drop spacing = 1.1 * interval)*  to ensure that the

this contradicts the pseudocode:

    dodequeue_result codel_queue_t::dodequeue(time_t now)

	...
	first_above_time_ = now + interval_;

section 4:

    To keep from
    making drops when it would starve the output link, CoDel makes
    another check before dropping to see if at least an MTU worth of
    bytes remains in the buffer. If not, the packet SHOULD NOT be
    dropped and,*currently*, CoDel exits the drop state.

what is the meaning of "currently" ? in the current experimental version?

---
(nit)

    In practice, this value is not known or measured*(*though see
    Section 6.2 
<https://tools.ietf.org/html/draft-ietf-aqm-codel-03#section-6.2>  for an application where interval is measured.

missing closing bracket

Section 5 / pseudocode: (all nits)

The variables are introduced as first_time_above, dropping, but are used 
as first_time_above_, dropping_ (with underscores at end).

---

*Packet** CoDelQueue::dequeue()

a pointer to current packet was introduced as packet_t

*--- double*  now = clock();;

time units were supposed to be of type time_t

*--- dodequeueResult*  r = dodequeue(now);

compare with: typedef struct { ... } *dodequeue_result* below (present 
in two routines)

---

dodequeueResult r = {*NULL, queue_t::dequeue() *};

shouldn't it be {queue_t::dequeue(), false} ?


Best Regards,
Polina


* note the reply by Dave Taht: 
http://www.ietf.org/mail-archive/web/aqm/current/msg01687.html

On 03/20/2016 04:35 PM, Wesley Eddy wrote:
> It looks like the WGLC feedback on the document body is incorporated, 
> so this is good.
>
> Is there a reason to stay with Informational and not Experimental like 
> we've done with PIE an d FQ-CoDel?
>
> Also, idnits has some problems with the references that should be 
> fixed (e.g. "NATAL2010" is probably supposed to be "NETAL2010"):
> https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-aqm-codel-03.txt