Hi Thanks for your review comments. Answers inline marked [IJ] /Ingemar > -----Original Message----- > From: Joel Halpern [mailto:jmh@xxxxxxxxxxxxxxx] > Sent: den 14 oktober 2017 21:11 > To: gen-art@xxxxxxxx > Cc: draft-ietf-rmcat-scream-cc.all@xxxxxxxx; ietf@xxxxxxxx; rmcat@xxxxxxxx > Subject: Genart last call review of draft-ietf-rmcat-scream-cc-11 > > Reviewer: Joel Halpern > Review result: Almost Ready > > I am the assigned Gen-ART reviewer for this draft. The General Area Review > Team (Gen-ART) reviews all IETF documents being processed by the IESG for > the IETF Chair. Please treat these comments just like any other last call > comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-rmcat-scream-cc-11 > Reviewer: Joel Halpern > Review Date: 2017-10-14 > IETF LC End Date: 2017-10-23 > IESG Telechat date: 2017-10-26 > > Summary: This document is almost ready for publication as an experimental > RFC. > > The reviewer hopes that the problem noted here are due to his mis-reading. > > Major issues: > The description of "a" after the pseudocode in section 4.1.2 states that > it is positive when qdelay is increasing, and negative when qdelay is > decreasing. However, a is the ratio of two evaluations of the function R > with different lags. And R is defined as the sum of products of entries > in the history vector. The history elements (qdelay_fraction_avg) are > always positive. So the terms in R are always positive. So a is always > positive? [IJ] You are absolutely right !, this is an error in the description. The variable a should actually be computed based on the vector qdelay_fraction_hist with the mean (DC component) removed. The corrected code would be (I changed a to a_t as it is a temporary variable. ===== # Compute the average of the values in qdelay_fraction_hist avg_t = average(qdelay_fraction_hist) # R is an autocorrelation function of qdelay_fraction_hist, # with the mean (DC component) removed, at lag K # The subtraction of the scalar avg_t from qdelay_fraction_hist # is performed element-wise. a_t = R(qdelay_fraction_hist-avg_t,1)/R(qdelay_fraction_hist-avg_t,0) . . ===== > > Minor issues: > There appears to be something odd with the mathematical expression for > the > autocorrelation function R or its usage. As written, with lag k the > function uses N-k terms. Which means that if the qdelay stays perfectly > constant, a will be N-1/N rather than 1 (i.e. 0.95). If this is > intentional, it would be good to say so. [IJ] I believe this I becomes clear with the correction above. It is possible to compute an unbiased estimate but it is not necessary for the function here. > > What does the text in section 3.1 reading "It is however necessary that > they [ sender and receiver] use the same clock frequency" mean? Times > are > exchanged. Frequencies are not. Is this intended to be a statement about > resolution? it appears to describe something that is not visible to the > protocol. As such, what happens if the requirement is not met and the > failure is not detected?" [IJ] OK. What about add something like "Failure to meet this requirement leads to malfunction in the SCReAM congestion control algorithm as the queue delay will not be estimated correctly." > > max_bytes_in_flight appears in the pseudocode in section 4.1.2.2, but > does > not appear in the list of state variables earlier in the document. [IJ] OK, will fix this > > Nits/editorial comments: > In the pseudocode in section 4.1.2, is the variable "a" really "a_t"? [IJ] Should be a_t > > In section 4.1.1.2 in the definition of min_cwnd, should there be a note > about the (admittedly unlikely) case where 2*MSS is less then > MIN_CWND? [IJ] actually min_cwnd should be replaced by MIN_CWND and min_cwnd should be removed from the document > > In the last paragraph of 4.1.2.2, is the new cwnd limited by the variable > min_cwnd as stated in the text, or the constant MIN-CWND as shown in > the > pseudocode? [IJ] Should be MIN_CWND in all places. min_cwnd should be removed. >