Rasool, Thanks for your detailed review. Please see the comments below and a new draft has been posted.
Regards, Rong On 3/21/16, 8:20 PM, "aqm on behalf of Rasool Al-Saadi" <[email protected] on behalf of [email protected]> wrote: >Dear all, > >Thank you for revising PIE Internet Draft as it has become much clearer >than previous versions. However, I have some comments regarding then new >draft: > >1) In page 12, section 5.2 (Departure Rate Estimation), > >> current_qdelay = queue_.byte_length() * >> PIE->avg_dq_time_/DQ_THRESHOLD; >> > >This code assumes that DQ_THRESHOLD of bytes are de-queued during >avg_dq_time , but this is not very accurate as we can de-queue >DQ_THRESHOLD + MSS during the measurement cycle (the condition is >PIE->dq_count_ > DQ_THRESHOLD). Are you trying to simplify the code or a >very accurate current_qdelay calculation is not required? >>>>>>>>>>>>>>>>RP: it is for simplification purpose and accurate latency >>>>>>>>>>>>>>>>calculation is not required. It is probability-based >>>>>>>>>>>>>>>>scheme, there are a lot of randomness built-in. > > >2) In the same section mentioned above, > >> * Upon a packet deque: >> >> if PIE->in_measurement_ == FALSE and queue.byte_length() > >> DQ_THRESHOLD: >> PIE->in_measurement_ = TRUE; >> PIE->measurement_start_ = now; >> PIE->dq_count_ = 0; >> >> if PIE->in_measurement_ == TRUE: >> PIE->dq_count_ = PIE->dq_count_ + deque_pkt_size; >> if PIE->dq_count_ > DQ_THRESHOLD then >> weight = DQ_THRESHOLD/2^16 >> PIE->avg_dq_time_ = (now-PIE->measurement_start_)*weight >> + PIE->avg_dq_time_*(1-weight); >> PIE->dq_count_=0; >> PIE->measurement_start_ = now > >This code works correctly first time when in_measurement_== FALSE then, >it will stay in measurement state even when there are no DQ_THRESHOLD >worth of bytes in the queue. So, PIE->avg_dq_time_ will be wrong as it >includes the time of filling the queue with DQ_THRESHOLD worth of bytes >plus the time of de-queuing it. The simple solution for that is just by >changing the last two lines of the code: > >> PIE->dq_count_=0; >> PIE->measurement_start_ = now > >By > PIE->in_measurement_ = FALSE; > >BTW, the code in the end of the daft is correct. >>>>>>>>>>>>>>>>>>RP: Pseudo code is correct, here is not the complete >>>>>>>>>>>>>>>>>>code, merely to explain the key part of the algorithm. I >>>>>>>>>>>>>>>>>>now add the else clause in to be clear. > > >3) In page 16, section 6 (Implementation Cost), the paragraph that >describes combing SFQ with PIE is not clear and confusing. > >"SFQ can also be combined with PIE to further improve latency for various >flows with different priorities. If the timestamp is used to obtain >queueing latency, PIE can be adopted directly to each individual queue. >If the latency is obtained via the deque rate calculation, we recommend >one PIE instance using the overall queue length divided by the overall >deque rate. Then the overall PIE->drop_prob_ is modified using each >individual queue divided by the maximum individual queue length: PIE- >>drop_prob_(i)=queue_.byte_length(i)/max(queue_.byte_length(i)). " > >I am not sure if I interpreted the paragraph correctly or not. It seems >that there are two cases when using PIE with SFQ scheduler : >1- When timestamp is used to calculate queue delay, each queue has a >separate drop probability calculation background process i.e. each queue >uses normal PIE instance. > >2- When deque rate calculation is used to calculate queue delay, here I >am completely confused about what does the text means and hope you >clearly it: > >"we recommend one PIE instance using the overall queue length divided by >the overall deque rate. Then the overall PIE->drop_prob_ is modified >using each individual queue divided by the maximum individual queue >length: PIE- >>drop_prob_(i)=queue_.byte_length(i)/max(queue_.byte_length(i))." > >For new drop_prob calculation, how can drop_prob_ be just a division of >queue lengths? > drop_prob_(i)=queue_.byte_length(i)/max(queue_.byte_length(i)) >>>>>>>>>>>>>>>>>>>>>>>>>RP: Let¹s say there are two flows that are queued into two sfq queues. One flow is an elephant flow and one flow is mice flow. Total latency is caused by the elephant flow. The above equation makes sure that the mice flow (whose queue length is zero, or very small amount) won¹t be dropped. Besides, since it is mice flow, it would be hard to measure its departure rate as the queue would never build up.qdelay > > >4) In page 20, the code of enque function: > >>//called on each packet arrival >> enque(Packet packet) { >> if (PIE->drop_prob_ == 0 && current_qdelay < QDELAY_REF >> && PIE->qdelay_old_ < QDELAY_REF) { >> PIE->burst_allowance_ = MAX_BURST; >> } > >According to the description and the code in section 4.4 (Burst >Tolerance), the condition should be > if (PIE->drop_prob_ == 0 && current_qdelay < QDELAY_REF/2 > && PIE->qdelay_old_ < QDELAY_REF/2) { >>>>>>>>>>>>>>>>>>>>>>>>>>Fixed... > >5) In pages 23 and 24, > >> //If the queue has been idle for a while, turn off PIE >> //reset counters when accessing the queue after some idle >> //period if PIE was active before >> if ( PIE->drop_prob_ == 0 && PIE->qdelay_old_ == 0 >> && queue_.byte_length() == 0) { >> PIE->active_ = INACTIVE; > >According to section 5.3 (Turning PIE on and off), the condition of >deactivating PIE should be: > >if (PIE->drop_prob_ == 0 and current_qdelay < QDELAY_REF/2 and > PIE->qdelay_old_ < QDELAY_REF/2) { >>>>>>>>>>>>>>>>>>>>>>>>>>RP: Fixed. > > >6) In page 26, > >> if ( PIE->dq_count_ >= DQ_THRESHOLD) { >And >> if (queue_.byte_length() >= DQ_THRESHOLD && > >Just to make the conditions consistence with section 5.2 (Departure Rate >Estimation), use '>' instead of '>=' or change the condition in the code >of section 5.2 to use '>=' instead of '>'. > >>>>>>>>>>>>>>>>>>>>>>>>>>RP: Fixed, changed in Section 5.2 > >Regards, >Rasool Al-Saadi > >> -----Original Message----- >> From: aqm [mailto:[email protected]] On Behalf Of internet- >> [email protected] >> Sent: Wednesday, 2 March 2016 7:16 AM >> To: [email protected] >> Cc: [email protected] >> Subject: [aqm] I-D Action: draft-ietf-aqm-pie-05.txt >> >> >> A New Internet-Draft is available from the on-line Internet-Drafts >>directories. >> This draft is a work item of the Active Queue Management and Packet >> Scheduling of the IETF. >> >> Title : PIE: A Lightweight Control Scheme To Address >>the Bufferbloat >> Problem >> Authors : Rong Pan >> Preethi Natarajan >> Fred Baker >> Filename : draft-ietf-aqm-pie-05.txt >> Pages : 26 >> Date : 2016-03-01 >> >> Abstract: >> Bufferbloat is a phenomenon where excess buffers in the network cause >> high latency and jitter. As more and more interactive applications >> (e.g. voice over IP, real time video streaming and financial >> transactions) run in the Internet, high latency and jitter degrade >> application performance. There is a pressing need to design >> intelligent queue management schemes that can control latency and >> jitter; and hence provide desirable quality of service to users. >> >> This document presents a lightweight active queue management design, >> called PIE (Proportional Integral controller Enhanced), that can >> effectively control the average queueing latency to a target value. >> Simulation results, theoretical analysis and Linux testbed results >> have shown that PIE can ensure low latency and achieve high link >> utilization under various congestion situations. The design does not >> require per-packet timestamp, so it incurs very small overhead and is >> simple enough to implement in both hardware and software. >> >> >> The IETF datatracker status page for this draft is: >> https://datatracker.ietf.org/doc/draft-ietf-aqm-pie/ >> >> There's also a htmlized version available at: >> https://tools.ietf.org/html/draft-ietf-aqm-pie-05 >> >> A diff from the previous version is available at: >> https://www.ietf.org/rfcdiff?url2=draft-ietf-aqm-pie-05 >> >> >> Please note that it may take a couple of minutes from the time of >>submission >> until the htmlized version and diff are available at tools.ietf.org. >> >> Internet-Drafts are also available by anonymous FTP at: >> ftp://ftp.ietf.org/internet-drafts/ >> >> _______________________________________________ >> aqm mailing list >> [email protected] >> https://www.ietf.org/mailman/listinfo/aqm > >_______________________________________________ >aqm mailing list >[email protected] >https://www.ietf.org/mailman/listinfo/aqm _______________________________________________ aqm mailing list [email protected] https://www.ietf.org/mailman/listinfo/aqm
