Subject: [aqm] Spencer Dawkins' Yes on draft-ietf-aqm-fq-codel-05: (with COMMENT)

From: "Spencer Dawkins" <[email-masked]>
Date: Wed, 16 Mar 2016 11:33:39 -0700

Spencer Dawkins has entered the following ballot position for
draft-ietf-aqm-fq-codel-05: Yes

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-aqm-fq-codel/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Very nice work. I have some nit-ish questions I hope you'll consider, but
nothing blocking.

This text in the Introduction,

   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.
   
introduces both CoDel and DDR, but they don't have references until
Section 1.3. Maybe move that forward in the doc?

Perhaps this text:

   FQ-CoDel stochastically classifies incoming packets into different
   queues by hashing the 5-tuple of IP protocol number and source and
   destination IP and port numbers, perturbed with a random number
   selected at initiation time (although other flow classification
   schemes can optionally be configured instead). 
   
would benefit from a forward reference to Section 4.1.1?

Just as a nit, you have an "is is" in Section 3.

In this text:

   After having selected a queue from which to dequeue a packet, the
   CoDel algorithm is invoked on that queue.  This applies the CoDel
   control law, 
   
"CoDel control law" hasn't been introduced, and isn't included in Section
1.2. Could you provide a reference or pointer?

In this text:

   This parameter can be set only at load time since memory has to be
   allocated for the hash table in the current implementation.
   
if "in the current implementation" refers to "can be set only at load
time", 

   This parameter can be set only at load time in the current
implementation,
   since memory has to be allocated for the hash table.
   
might be clearer.

I wonder if 

   while CoDel itself can take a while
   to respond, fq_codel doesn't miss a beat.
   
is clear to non-native English language readers?

In this text:

   In the presence of queue management schemes that contain latency
   under load, 
   
"contain" means something like "limit" right? That wasn't what I
understood in my first parsing attempt.

I'm confused by this text:

   o  Packet fragments without a layer 4 header can be hashed into
      different bins than the first fragment with the header intact.
      This can cause reordering and/or adversely affect the performance
      of the flow.  Keeping state to match the fragments to the
      beginning of the packet, or simply putting all fragmented packets
      into the same queue, are two ways to alleviate this.
      
Is this "all fragmented packets", or "all packet fragments"?

If it's "all fragmented packets", don't you have to reassemble the
fragments? 

If it's "all packet fragments", the packet fragments would all be
transmitted in order, but could the fragments with headers and without
headers of a single packet be reordered?

But either way, I have a question :-) ...

Do we say things like 

   In this document we have documented the Linux
   implementation in sufficient detail for an independent
   implementation, and we encourage such implementations be widely
   deployed.

in Experimental RFCs?

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Thu, 17 Mar 2016 18:07:21 +0100

"Spencer Dawkins" <[email-masked]> writes:

> Very nice work. I have some nit-ish questions I hope you'll consider,
> but nothing blocking.

Hi Spencer

Thank you, and thank you for your comments. I've updated the text to
account for them; the updated version is currently available here:
https://kau.toke.dk/ietf/draft-ietf-aqm-fq-codel-06.html (or .txt), and
I'll post it to the IETF tracker once I'm sure I have covered all the
review comments.

> Section 1.3. Maybe move that forward in the doc?

This has been fixed.

> would benefit from a forward reference to Section 4.1.1?

You're right; added the forward reference.

> Just as a nit, you have an "is is" in Section 3.

Yup, you're not the first one to notice; fixed :)

> 1.2. Could you provide a reference or pointer?

I've updated the text to explain what the control law does, and point to
the CoDel draft. The full paragraph now reads:

    After having selected a queue from which to dequeue a packet, the
    CoDel algorithm is invoked on that queue. This applies the CoDel
    control law, which is the mechanism CoDel uses to determine when to
    drop packets (see [I-D.ietf-aqm-codel]). As a result of this, one
    or more packets may be discarded from the head of the selected
    queue, before the packet that should be dequeued is returned (or
    nothing is returned if the queue is or becomes empty while being
    handled by the CoDel algorithm).

>    since memory has to be allocated for the hash table.

I agree. Fixed.

> is clear to non-native English language readers?

That was already changed to read "while CoDel itself can take a while to
respond, FQ-CoDel reacts almost immediately".

> understood in my first parsing attempt.

I see what you mean. Changed 'contain' to 'limit'.

> headers of a single packet be reordered?

It's the latter. Updated the text to read "Keeping state to match the
fragments to the beginning of the packet, or simply putting all packet
fragments (including the first fragment of each fragmented packet) into
the same queue, are two ways to alleviate this."

>    implementation, and we encourage such implementations be widely

Apparently not, as others have pointed out. Fixed :)

-Toke