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

[Andrew Mcgregor <[e-mail]>]

Thank you Polina.

We have incorporated most of these (sometimes with slightly different
solutions than you suggested), and a new draft will be out very shortly.  I
felt that by far the most important was the different way the Linux
implementation addressed re-entry to the dropping state, and the pseudocode
now does exactly that.

Andrew

On 22 March 2016 at 08:10, Polina Goltsman <[e-mail]>
wrote:

> 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 of section 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
>
>
>
> _______________________________________________
> aqm mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/aqm
>
>


-- 
Andrew McGregor | SRE | [e-mail] | [phone]