Subject: [aqm] WGLC for PIE drafts

From: Wesley Eddy <[email-masked]>
Date: Tue, 1 Dec 2015 15:51:46 -0500

This is the start of a working group last call for the PIE specification:
https://tools.ietf.org/html/draft-ietf-aqm-pie-03

along with the DOCSIS-PIE description as Informational:
https://tools.ietf.org/html/draft-ietf-aqm-docsis-pie-01

Please send any comments on the documents by the end of December. There 
is an open question on the PIE specification of whether it should be 
standards track or not.  I think we've punted on this a bit in the past, 
and need to understand the consensus on now.

Specific questions for the working group that we / chairs need clear 
feedback on:
- Are the documents ready to publish as RFCs?
- Should the PIE specification be Proposed Standard or Experimental?

---

From: "Ilpo Järvinen" <[email-masked]>
Date: Tue, 22 Dec 2015 23:56:41 +0200 (EET)

On Tue, 1 Dec 2015, Wesley Eddy wrote:

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

---

From: "Ilpo Järvinen" <[email-masked]>
Date: Wed, 23 Dec 2015 17:02:30 +0200 (EET)

> correct or is there some corner-case where drop_prob_ > 0?

Another hazard with this unconditional subtracting is an underflow. It 
seems that at least the example code in Section 11 and 12 limit the 
substraction of T_UPDATE to positive burst_allowance which avoids both 
issues but it would be much better to have that in 4.4 code fragment
too.

-- 
 i.