Subject: Re: [tcpm] [EXTERNAL] hystart++ -03 review

From: Praveen Balasubramanian <[email-masked]>
Date: Tue, 14 Jul 2020 01:04:59 +0000

Hi Mark,

Sorry for the delayed response. Thanks for the review. Answers inline. Most of the changes to the text have been incorporated in the WG version of the document. 
https://tools.ietf.org/html/draft-ietf-tcpm-hystartplusplus-00

Thanks

-----Original Message-----
From: tcpm <[email-masked]> On Behalf Of Mark Allman
Sent: Tuesday, May 12, 2020 12:54 PM
To: Extensions <[email-masked]>
Subject: [EXTERNAL] [tcpm] hystart++ -03 review

I read version -03 of the HyStart++ document and my view differs from many, I guess.  IMHO, this document is not ready for adoption.

The draft is pretty anemic and I am not sure there is enough there to actually judge whether it is or is not a good idea.  It seems like perhaps the document could get there, but at this point I think the authors have some work to do.  Review below.

Section 1:

  - Nit: RFC 793 does not define slow start, remove ref.  (Could add
    a ref to original Jacobson paper.)

Good catch. Fixing to just refer to RFC 5681 instead of both RFC 793 and RFC 5681. 

  - HyStart++ is "widely deployed", but is it widely used?  Is it
    enabled or just there for the enabling?  The document makes it
    sound like we have a lot of experience with this somehow, but
    none of that is shared (see below).

Widely deployed was intended to imply widely used. Created a section "Deployments and Performance Evaluations" so this text has been changed. 

  - Nit: Spell out "LSS" on first use.

LSS is spelled out in the first use in the Abstract. Happy to repeat it in the Introduction. 

Section 3:

  - Nit: Totally fine to crib the definitions from RFC 5681, but
    it'd be good form to note that is where they came from.

Sounds good. We now cite RFC 5681 in this section. 

  - Nit: Also, since you use rwnd in one of the definitions and
    you're copying things, you might as well copy that one, too.

Done.

Section 4:

  - There is so much unexplained magic in here.  When setting
    RttThresh, what is magic about 1/8-th of the lastRoundMinRtt?
    Why clamp this between 4msec and 16msec?  Would that really be
    reasonable for all networks?

    We need magic numbers sometimes and as long as they're
    resasonably chosen and motivated we have to live with having
    them around.  But, the draft really needs to explain these
    choices, how they were made and how broadly applicable they
    might be.  Further, the document suggests one MAY vary the
    constants, but it'd be great if it gave some sort of advice on
    how one might decide if different constants were OK or not OK.

  - The algorithm (series of equations) is here and clear.  But, the
    document really needs to give us a flavor of the why.  Talk us
    through what is happening and why in here, please.

Added descriptions of what the implications are for tuning these constants and what role they play in the heuristic in the new section "Tuning constants". 

General / meta:

  - The draft offers a little intuition on why this is a good idea
    (i.e., to prevent massive slow start over-shoot).  But, there is
    no sketch of evidence that shows it is effective.  I see this
    was previously mentioned on the mailing list and Praveen said:

      We presented lab measurement data at the last IETF. We are in
      the process of doing some A/B experiments at scale with real
      world workloads. We did fairness comparisons in our lab
      measurements between Hystart and non-Hystart and we didn't see
      any problems.

    The bar ought to be higher than "didn't see any problems".  It
    is great that there is some testing done and planned.  I'd like
    to see this all sketched in the document itself.  I am not even
    close to convinced that this is a good idea.

No, the bar is not we didn't see any problems :). That answer was specifically scoped to a question around fairness. We did not observe any fairness issues in the measurements. 
The lab and prod data has been presented in multiple IETF meetings. Added data to the "Deployments and Performance Evaluations" section. 

  - In some sense, this idea makes TCP more conservative (i.e., we
    stop exponential increase sooner than RFC5681 would suggest).
    So, the bar is likely not huge for saying this is "OK".  But, I
    think we should have some notion that this helps *something*
    before agreeing to suggest it as somehow a reasonable thing to
    do in an RFC.  Right now this document doesn't concretely show
    that HyStart++ is useful in any way.

Correct more conservative. Added text in the draft to call out the improvements.