Last Call Review of draft-ietf-aqm-fq-codel-05
review-ietf-aqm-fq-codel-05-genart-lc-davies-2016-03-09-00

  
  
    I am the assigned Gen-ART reviewer for this draft. The General Area
    


    Review Team (Gen-ART) reviews all IETF documents being processed
    


    by the IESG for the IETF Chair.  Please treat these comments just
    


    like any other last call comments.
    





    For more information, please see the FAQ at
    







<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>

.
    





    Document:
    draft-ietf-aqm-fq-codel-05.txt


    Reviewer:
    Elwyn Davies


    Review Date:
    2016/03/06


    IETF LC End Date:
    2016/03/17


    IESG Telechat date: (if known) 
    2016/03/17





    Summary:
    


    Almost ready.  There are a couple of minor issues that are barely
    above the level of nits plus a fair bit of editorial work.





    I notice that the draft is on the IESG agenda on the day that last
    call ends.  If the authors wish to sort out the nits before the end
    of last call, I will be happy to work with them on this.





    Major issues:
    


    None.





    Minor issues:
    


    Treatment of packets that don't fit into the hashing classification
    scheme:  The default FQ-CoDel hashing mechanism uses the
    protocol/addresses/ports 5-tuple, but there will be packets that
    don't fit this scheme (especially ICMP).  There is no mention of
    what the classification would do with these packets.  I guess that
    one extra queue would probably suffice to hold any such outliers,
    but it would be wise to say something about how the packets from
    this/these queue(s) would be treated by the scheduler.  It might
    also be useful to say something about treatment of outliers in other
    classification schemes, if only to say that the scheme used needs to
    think about any such outliers. 





    s6.2, last para:  The proposal to add flow related marking to
    encapsulated packets needs to come with a very well exposed security
    and privacy health warning.. [It occurs to me that it might be
    possible to confine these markings to out of band notifications
    within the originating host which would allow fq_codel or similar to
    Flow Queue the packets getting them into a sensible order on the
    outgoing link.  Once the packets had been ordered in this way, a
    subsequent box (maybe an ADSL modem or suchlike) linking a home
    environment to the outside world could work purely on source
    address, thereby interspersing the packets from different hosts onto
    the external link but not needing to reorder the packets from each
    individual host.  Not sure if this is a sensible idea but it looks
    like a way to avoid the privacy issues.]





    s11:  Internet Drafts are not scientific papers as such and do not
    usually have a Conclusions section.  I think it would be useful to
    move the paragraph in s11 as is to s1.  Since this draft is targeted
    for Experimental RFC status, it would be useful to suggest (maybe in
    s7) that experiments along the lines noted in s7 might be carried
    out and there results reported (where?  AQM WG?).  Given the
    developments with Cake, I am not sure whether the authors expect
    this version of FQ-CoDel to make it onto standards track or BCP, but
    to set out some sort of expected trajectory is desirable. 





    Nits/editorial comments:
    


    General: s/i.e./i.e.,/, s/e.g./e.g.,/





    Draft Title: The acronym CoDel with an uppercase D is used
    everywhere else in the document - Suggest the title should be
    FlowQueue-CoDel





    Abstract:   Need to expand AQM. [DNS and and CPU are 'well-known' -
    

http://www.rfc-editor.org/materials/abbrev.expansion.txt

]





    General: It would be helpful to capitalize Quantum throughout (or at
    least from s3 onwards)  to emphasise that it is a configured value. 
    Likewise Interval and Target parameters.  Maybe also Flow and Queue
    as they a defined terms.





    s1, para 1: It would be helpful to give the full title
    (FlowQueue-CoDel), expand AQM (again), provide a refernece
    explaining the term bufferbloat and give references for AQM, basic
    CoDel, DRR and the ns2/ns3 network simulators.





    I would think one or both of the following would be useful (long
    term stable) refs for bufferbloat (the first is useful because the
    article is publically available rather than needing a sub to IEEE or
    ACM):





    Jim Gettys. 2011. Bufferbloat: Dark buffers in the Internet. IEEE
    Internet Comput. 15, 3 (2011), 95–96.
    DOI:

http://dx.doi.org/10.1109/MIC.2011.56

 (freely available at
    

http://www.bufferbloat.net/attachments/27/IC-15-03-Backspace.pdf

)





    and/or 


    Jim Gettys and Kathleen Nichols. 2012. Bufferbloat:Dark buffers in
    the Internet. Commun. ACM 55, 1 (2012), 1–15.
    DOI:

http://dx.doi.org/10.1145/2063176.2063196





    Suggest:


    OLD:


       The FQ-CoDel algorithm is a combined packet scheduler and AQM


       developed as part of the bufferbloat-fighting community effort. 
    It


       is based on a modified Deficit Round Robin (DRR) queue scheduler,


       with the CoDel AQM algorithm operating on each queue.  This
    document


       describes the combined algorithm; reference implementations are


       available for ns2 and ns3 and it is included in the mainline
    Linux


       kernel as the fq_codel queueing discipline.





    NEW:


       The FlowQueue-CoDel (FQ-CoDel) algorithm is a combined packet
    scheduler and 


       Active Queue Management (AQM) [RFC 3168] algorithm


       developed as part of the bufferbloat-fighting community effort
    (see 

http://www.bufferbloat.net

).  It


       is based on a modified Deficit Round Robin (DRR) queue scheduler
    [DRR][DRRPP],


       with the CoDel AQM algorithm operating on each queue.  This
    document


       describes the combined algorithm; reference implementations are


       available for the ns2 (

http://nsnam.sourceforge.net/wiki

) and 


       ns3 (

https://www.nsnam.org/wiki

) network simulators, and it is 


       included in the mainline Linux kernel as the fq_codel queueing
    discipline.


    END





    s1.2, definition of Flow: s/protocol/protocol number/; also mac is
    ambiguous - s/mac address/media access control (MAC) address/





    s1.2, definitions of Flow and Queue: To make the mentions of hash in
    the definition of Queue comprehensible, it would be sensible to
    mention 'hash' in the definition of flow.  Suggest adding:


        The identifier(s) of a flow are mapped to a hash code used
    internally in FQ-CoDel to organize the packets into Queues.





    s1.2: Probably worthwhile adding the references for CoDel and DRR to
    the definitions.





    s1.2: It would help with subsequent understanding to provide
    definitions of the Interval and Target parameters of the CoD


    el scheme.  Suggest:


    ADD:


    Interval: Characteristic time period used by the control loop of
    CoDel to age the value of the minimum sojourn time of packets in a
    Queue (i.e., the time spent by the packet in the local Queue) that
    is used to indicate when a persistent Queue is developing (see
    Section 4.3 of [CODELDRAFT]).





    Target: Setpoint value of the minimum sojourn time of packets in a
    Queue used as the target of the control loop in CoDel (see Section
    4.4 of [CODELDRAFT]).


    END





    s1.3, para 1: It would be helpful to expand SQF: s/SQF/Shortest
    Queue First (SQF)/





    s1.3, para 1: s/differently than/differently from/ (the latter is
    good for both US and British English)





    s1.3, para 2: Suggest starting the para with 'By default,' so that
    the 'although' bit is not a surprise.





    s1.3, para 2:  Adding a reference for the CoDel algorithm would
    help.  In particular, linking to
    

http://queue.acm.org/appendices/codel.html

 as well as the basic
    article would be helpful - I am not sure if the CoDel article is
    available for free to all but the appendix seems to be.





    s1.3, para 4: s/current implementation/Linux implementation at the
    time of writing/  





    s1.3, para 5: s/rather than a packet-based./rather than
    packet-based./





    s1.3, para 6 (next to last para): 







However, in practice many things that are
   beneficial to have prioritised for typical internet use (ACKs, DNS
   lookups, interactive SSH, HTTP requests, ARP, RA, ICMP, VoIP) _tend_
   to fall in this category, which is why FQ-CoDel performs so well for
   many practical applications.





    I am doubtful as whether mentioning ARP, RA and ICMP in this
    statement is appropriate since they don't fit into the default flow
    classification scheme, and are not really flows as such.  As
    mentioned in Minor Issues above some discussion of non-flow packets
    is needed IMO.  They could be left out here without problem I think.





    s3.1, para 1: s/buckets are configurable/buckets is configurable/





    s3.1, para 3: s/This number is is/This number is/





    s3.1, para 3: 


    OLD:


    until the number of credits runs into the negative


    NEW:


    until the value of _byte credit_ becomes negative


    END


    What about _byte_credit_ == 0?  (Just asking!  Did possibly think
    that if the queue has the same number of bytes as the credit, it
    might be possible for the queue to jam in the active list while
    empty.)





    s3.1,para 4: s/fairness queueing/fairness queueing scheme/





    s3.1, last para: s/flow-building queues/queues that build a backlog/





    s4.1, para 2: A reference for Jenkins hash functions would be
    desirable.  It seems the best we can probably do is Jenkins' web
    site: 

http://www.burtleburtle.net/bob/hash/doobs.html





    s4.1:  The basic Jenkins has used in the Linux scheduler code
    appears to be targeted at IPv4 32 bit addresses.  Technically, the
    abstract definition doesn't need to worry about the lengths of
    addresses, but does anything special need to be said about IPv6?  I
    couldn't see in the Linux code where IPv6 is dealt with. 
    Furthermore, looking at the code a bit more closely, I am not sure
    how the 112 bits (32 + 32 + 16 + 16 + 16) of the 5-tuple are pushed
    into the hashing algorithm which has an input 'bandwidth' of 64
    bits.  Again this doesn't desperately matter here but, if I
    understand correctly, the calculations in s5.3 of hash collision
    probabilities reported in para 2 of s5.3 relate to the Jenkins
    update3 has algorithm used in Linux.  This isn't explicitly stated. 
    I am not sure if the probabilities would be different if the number
    of bits in the 5-tuple was greater - presumably this depends to some
    extent on how the Jenkins algorithms are used.  The core Linux
    algorithm just uses the 'final' part of the algorithm.  According to
    the Jenkins comments, one probably ought to use the 'mix' part to
    combine additional bits.  Thoughts?





    s4.1, para 2:  I am not sure that 'permuted by a random value' is
    very clear, and it doesn't explain why this is done.


    How about 'salted by modular addition of a random value'?  It would
    also be helpful to note here that this is done to mitigate a
    possible DoS attack that could occur if the hash is externally
    predictable and point to the note about this in the Security
    Considerations (s8).





    s4.1.1, para 1: s/mac address/MAC address/; s/diffserv/diffserve
    codepoint values/


    [I am not sure what firewall specific markings might be - any
    references?]





    s4.1.1, para 2:  Suggest: 


    OLD:


    An alternative to changing the classification scheme is to perform


       decapsulation prior to hashing.


    NEW:


    An addition to the default classification scheme is to perform


       decapsulation of tunnelled packets prior to hashing on the
    5-tuple in the encapsulated.


    END





    Figure in s4.2: Needs a caption and a Figure number.  It is also
    somewhat incomplete as a state diagram.  Both New and Old states
    have actions that result from both Arrival/Enqueue and Dequeue
    events, and there are 'loopback' actions when there are 
    Arrival/Enqueue and Dequeue events that occur in both New and Old
    states when the conditions are not satisfied.





    s5.2.1, para 1:







to ensure that the measured minimum delay does not become too stale.





    Without detailed knowledge of CoDel this didn't read very easily -
    it is taken out of context from the CoDel draft.


    Suggest:


       to  ensure that the minimum sojourn time of packets in a queue
    used as an estimator by the CoDel control algorithm is a relatively
    up-to-date value.





    s5.2.1, para 1: s/It SHOULD be set on the order/It SHOULD be set to
    be on the order/





    s5.2.3, para 2: s/10GigE/10 Gigabit Ethernet/





    s5.2.4, para 2: Expand TSO (TCP Segmentation Offload). TSO probably
    could do with a reference (Freimuth below should work).





    s5.2.4, para 2: I think GRO-enabled should probably be GSO-enabled -
    whichever it is, it needs to be expanded (Generic Segmentation
    Offload?).   There is also a web page for GSO
    (

https://lwn.net/Articles/188489/

)





     s5.2.5, para 2 and s4.1, para 2: It would be sensible to use a
    common term for 'initialisation time' and 'load time'.





    s5.2.6, title and body: Need to expand ECN on first use.  Also
    s/thus unresponsive/thus the number of unresponsive/ 





    s5.2.7: Expand CE (Congestion Encountered) on first occurrence.





    s5.2.7, para 1: Expand DCTCP acronym and provide reference
    [Alizadeh] below and/or [draft-ietf-tcpm-dctcp]....thus..


    OLD:




   This parameter enables DCTCP-like processing to enable CE marking ECT
   packets at a lower setpoint than the default codel target.


    NEW:




   This parameter enables Date Centre TCP (DCTCP)-like processing resulting in 
   CE (Congestion Encountered) marking on ECN-Capable Transport (ECT)
   packets [RFC3168] starting at a lower sojourn delay setpoint than the default CoDel 
   Target. Details of DCTCP can be found in [Alizadeh2011] and [I-D.draft-ietf-tcpm-dctcp]. 


    END





    s5.3, para 2: see the comments on s4.1 above regarding the link
    between probabilities and address lengths.





    s5.3, para 3: Need to expand Cake acronym and providepointer to
    Section 7 which expalisn about Cake.





    s5.4: Is it possible to add some comments to the codel_vars
    structure?





    s5.5, para 2: s/resolution below the target/resolution significantly
    finer than the CoDel Target setting/





    s5.6: Need to expand SFQ, WFQ and QFQ.  There is a reference for SFQ
    below [McKenney]





    s5.7, last para: s/doesn't miss a beat/reacts almost immediately/
    (Original is rather too colloquial!)





    s6.1, last para: s/classification/service classification/





    s7: I might add 'Checking behaviour when two or more instantiations
    of FQ-CoDel are used in series.'





    s12.2:  Various of the informative references could do with
    completion.  I think the referecnes below are correct:




Kathleen Nichols and
      Van Jacobson. 2012. "Controlling Queue Delay". 

Queue

 10, 5
      (May 2012), 20. DOI:

http://dx.doi.org/10.1145/2208917.2209336


    M. Shreedhar and George Varghese. 1996. "Efficient fair queuing
    using deficit round-robin". IEEE/ACM Trans. Netw. 4, 3 (June 1996),
    375–385. DOI:

http://dx.doi.org/10.1109/90.502236





    M.H. MacGregor and W. Shi. 2000. "Deficits for bursty
    latency-critical flows: DRR++". In Proceedings IEEE International
    Conference on Networks 2000 (ICON 2000). Networking Trends and
    Challenges in the New Millennium. IEEE Comput. Soc, 287–293.
    DOI:

http://dx.doi.org/10.1109/ICON.2000.875803







Y. Gong, D. Rossi,
      C. Testa, S. Valenti, and M.D. Taht. 2013. "Fighting the
      bufferbloat: On the coexistence of AQM and low priority congestion
      control". In 

2013 IEEE Conference on Computer Communications
        Workshops (INFOCOM WKSHPS)

. IEEE, 411–416.
      DOI:

http://dx.doi.org/10.1109/INFCOMW.2013.6562885


    Giovanna Carofiglio and Luca Muscariello. 2012. "On the Impact of
    TCP and Per-Flow Scheduling on Internet Performance". IEEE/ACM
    Trans. Netw. 20, 2 (April 2012), 620–633.
    DOI:

http://dx.doi.org/10.1109/TNET.2011.2164553





    The following are extra references that I suggested above:




P.E. McKenney. 1990.
      Stochastic fairness queueing. In 

Proceedings. IEEE INFOCOM
        ’90: Ninth Annual Joint Conference of the IEEE Computer and
        Communications Societies@m_The Multiple Facets of Integration

.
      IEEE Comput. Soc. Press, 733–740.
      DOI:

http://dx.doi.org/10.1109/INFCOM.1990.91316




Doug Freimuth et al.
      2005. Server network scalability and TCP offload. In 

USENIX
        Annual Technical Conference

. USENIX Association, 209–222.







Mohammad Alizadeh et
      al. 2011. Data center TCP (DCTCP). 

ACM SIGCOMM Comput. Commun.
        Rev.

 41, 4 (October 2011), 63–74.
      DOI:

http://dx.doi.org/10.1145/2043164.1851192