On Tue, 1 Dec 2015, Wesley Eddy wrote:
> This is the start of a working group last call for the PIE specification:
> https://tools.ietf.org/html/draft-ietf-aqm-pie-03
>
> Please send any comments on the documents by the end of December.
I think that draft-ietf-aqm-pie-03 still requires significant
improvements. There are huge gaps and corners in the algorithm to cover
before it would be implementable based on this given specification.
Comments below. Please note that although I use question mark here and
there in the comments, but I don't necessarily expect you to answer them
to me. It's just to indicate that either something that seems unclear/open
in the ID as is (meant to point out that perhaps the ID could be clearer
on those particular points) or that I just haven't gone through how the
internal variables could behave under the situations highlighted. In the
latter cases, the draft would obviously be fine but I just didn't know
the internals deep enough to remove any doubt.
Section 4.2:
- alpha/beta explained only in the Section 6 ("Implementation Cost") and
in code Section 11. Yet they're used and there's discussion about changing
them from unspecified to something already in 4.2.
- Second bullet claims to auto-tune alpha/beta but it only plays with
drop_prob_, seems odd. The code for this bullet has significant difference
to code in Section 11 as here the drop_prob_ is used as in-place replaced
variable, whereas in 11 there's intermediate variable p. Isn't this
difference going to affect the end result too? What is the preferred way
to implement this part?
- Third bullet code uses p instead of drop_prob_
- ID states: "The drop probability is a value between 0 and 1." this is
not enforced by the 4.2 section code fragments nor MUST as is. The code in
Section 11 enforces it (I read in-order and started to wonder how PIE
ends to drop_prob_==0 state while reading the burst allowance Section,
only from Section 11 I later found out that the code MUST enforce it
which wasn't obvious earlier).
- It's somewhat unclear to me if decay *0.98 should eventually
result in drop_prob_ becoming zero which unfortunately would depend on
used arithmetic precision? Or is the PI control law reducing drop_prob_
to a negative value with enforcing drop_prob_=0..1 going to dominate this
part of the operation always?
- In the last paragraph I'd start the third sentence with "If the target
latency is reduced, e.g. for data-center use, " to make the configurable
knob context change more obvious as now it traps the reader by first
talking about cut+increase and then about increasing both alpha and beta
which seems contradicting at that point and may make the reader to stop
there like I did (only after completing the sentence this contradiction is
resolved by telling the reader that it's actually about different tunable
knob).
Section 4.4:
- MAX_BURST works only with T_UPDATE granularity (that is, if there's
anything left in burst_allowance the whole update cycle is allowed even
if burst_allowance < T_UPDATE). Is this intentional? Would it be good to
mention if so (although the given default 15ms and 150ms config is not
affect)? If this granularity thing is intentional, couldn't the enqueue
path then be simpler by simply setting drop_prob_ to zero if there's burst
allowance left (and renaming the existing drop_prob_ used in 4.2
calculations e.g. to pi_drop_prob_ where from the drop_prob_ would
be set if burst quota is exhausted already)?
- If there are no enqueue events, burst_allowance is being reduced by
T_UPDATE timer. Could it occur first enqueue after idle sees
burst_allowance <= 0 but non-zero drop_prob_? I suppose this might not be
a problem as idle period would very likely mean that drop_prob_ is also 0
so the random drop won't drop it regardless of burst_allowance, is this
correct or is there some corner-case where drop_prob_ > 0?
Section 5.1:
- Should add ECN reference
Section 5.2:
- Variable naming issue: measurement_start_ vs start_
- "...latched value of depart_rate..." what is this depart_rate, should it
use dq_rate_ instead?
Section 5.3:
- Is the second burst_allowance_ = MAX_BURST required as PIE_active_ is
toggled FALSE? Oh, I see now (while writing this down here)... It's needed
to enforce "enqueue packet" (although more logical would be to use
PIE_active_ there though)?
- Does MAX_BURST perhaps need to be configured to a smaller value due to
PIE possibly being OFF for the initial onset of congestion?
References
- RFC 2309 missing, do you want to refer to that or to the newer one?
Section 11:
- QDELAY_REF = 16ms vs 15ms
- T_UPDATE = 16ms vs 15ms
- in enque: PIE->burst_allowance_ is checked twice, first with < 0 and
then with <= 0
- drop_early: "Safeguard" appear first here in "basic PIE pseudo code"?
- calculate_drop_prob: p used as temporary variable and scaling used is
not consistent with Section 4.2.
Section 12: Unfortunately I didn't have time to go through this Section.
I might go through that one in detail but I might only have time for that
after New Year.
Nits:
- Section 1: D in RED is Detection not Discard.
- Section 5.3: IT -> It
- enque/deque (and variants) -> enqueue/dequeue
- Variable names sometimes without _ in the end and sometimes with it.
- ex: -> e.g.
- last_timestampe_ -> last_timestamp_
--
i.
_______________________________________________
aqm mailing list
aqm@ietf.org
https://www.ietf.org/mailman/listinfo/aqm
