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

[Toke Høiland-Jørgensen <[e-mail]>]

"Spencer Dawkins" <[e-mail]> writes:

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> 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.


> 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?

This has been fixed.

> 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?

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 :)

> 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?

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).

> 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 agree. Fixed.

> 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?

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

> 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 see what you mean. Changed 'contain' to 'limit'.

> 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?

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."

> 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?

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

-Toke