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

[Ingemar Johansson S <[e-mail]>]

Hi
Thanks for your review comments. Answers inline marked [IJ]

/Ingemar

> -----Original Message-----
> From: Joel Halpern [mailto:[e-mail]]
> Sent: den 14 oktober 2017 21:11
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]
> 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. 
>