Subject: Genart last call review of draft-ietf-rmcat-scream-cc-11

From: Joel Halpern <[email-masked]>
Date: Sat, 14 Oct 2017 12:11:06 -0700

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?

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.

     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?"

    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.

Nits/editorial comments:
    In the pseudocode in section 4.1.2, is the variable "a" really "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?

    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?

---

From: Ingemar Johansson S <[email-masked]>
Date: Sun, 15 Oct 2017 19:34:58 +0000

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

/Ingemar

> -----Original Message-----
> From: Joel Halpern [mailto:[email-masked]]
> Sent: den 14 oktober 2017 21:11
> To: [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked]
> Subject: Genart last call review of draft-ietf-rmcat-scream-cc-11
> 
> Reviewer: Joel Halpern
> 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.
> 
[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)
.
.
=====

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

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

>     not appear in the list of state variables earlier in the document.
[IJ] OK, will fix this
>     In the pseudocode in section 4.1.2, is the variable "a" really "a_t"?
[IJ] Should be a_t
>     about the (admittedly unlikely) case where 2*MSS is less then
[IJ] actually min_cwnd should be replaced by MIN_CWND and min_cwnd should be removed from the document
>     min_cwnd as stated in the text, or the constant MIN-CWND as shown in
[IJ] Should be MIN_CWND in all places. min_cwnd should be removed.