Re: [Gen-art] Genart last call review of draft-ietf-rmcat-scream-cc-11

Joel, thanks for your review. Ingemar, thanks for addressing Joel’s comments. I have entered a No Objection ballot.

Alissa

> On Oct 15, 2017, at 3:34 PM, Ingemar Johansson S <ingemar.s.johansson@ericsson.com> wrote:
> 
> Hi
> Thanks for your review comments. Answers inline marked [IJ]
> 
> /Ingemar
> 
>> -----Original Message-----
>> From: Joel Halpern [mailto:jmh@joelhalpern.com]
>> Sent: den 14 oktober 2017 21:11
>> To: gen-art@ietf.org
>> Cc: draft-ietf-rmcat-scream-cc.all@ietf.org; ietf@ietf.org; rmcat@ietf.org
>> 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. 
>> 
> 
> _______________________________________________
> Gen-art mailing list
> Gen-art@ietf.org
> https://www.ietf.org/mailman/listinfo/gen-art