Early Review of draft-briscoe-docsis-q-protection-02
review-briscoe-docsis-q-protection-02-tsvart-early-westerlund-2022-02-18-00
Request | Review of | draft-briscoe-docsis-q-protection-01 |
---|---|---|
Requested revision | 01 (document currently at 07) | |
Type | Early Review | |
Team | Transport Area Review Team (tsvart) | |
Deadline | 2022-02-18 | |
Requested | 2022-01-28 | |
Requested by | Adrian Farrel | |
Authors | Bob Briscoe , Greg White | |
I-D last updated | 2022-02-18 | |
Completed reviews |
Tsvart Early review of -02
by Magnus Westerlund
(diff)
|
|
Comments |
This document has been present for publication as an Independent stream RFC. That means it is outside the IETF. The Independent Stream Editor is keen to receive a review with two aspects: - Does this document overlap with or otherwise infringe upon any IETF work? - Does this document read well to someone with Transport and Congestion control background? |
|
Assignment | Reviewer | Magnus Westerlund |
State | Completed | |
Request | Early review on draft-briscoe-docsis-q-protection by Transport Area Review Team Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/tsv-art/8eL0ep5Xx30MiAqgCsGn3_u0eIg | |
Reviewed revision | 02 (document currently at 07) | |
Result | Ready w/issues | |
Completed | 2022-02-18 |
review-briscoe-docsis-q-protection-02-tsvart-early-westerlund-2022-02-18-00
To first attempt to answer the review request questions. The Independent Stream Editor is keen to receive a review with two aspects: "- Does this document overlap with or otherwise infringe upon any IETF work?" It is clearly related to the L4S work in TSVWG. However, as it describes a particular implementation of an queue protection algorithm by DOCSIS I don't see it infringing upon any IETF work. "- Does this document read well to someone with Transport and Congestion control background?" It is understandable with a few exception that I will comment on below when clarity can be improved. In general I think it is close to being ready for publication. Just some issues that needs to be addressed. 1. Section 4.1: AGING = pow(2, (LG_AGING-30) ) * T_RES // lg([B/s]) to [B/T_RES] So the -30 is here is instead of dividing by 1E9? How much does the rounding error of ~7% impact? To my understanding the aging value will be smaller than intended with those 7%. 2. Section 4.1: NBUCKETS = pow(2, BI_SIZE); // No. of non-default buckets MASK = NBUCKETS-1; // convenient constant, filled with ones This appears to be a calculation that is based on that NBUCKETS and MASK are int and not floating point values? I would also note that MASK will not be filled with 1, only have NBUCKETS of ones at the lower bits using integer arithmetic. Does this document need clear conventions for when floating point is used versus integer arithmetic? 3. Section 4.1: // Minimum marking threshold of 2 MTU for slow links [ns] FLOOR = 2 * 8 * MAX FRAME SIZE * 10^9 / MAX RATE; Should not MAX FRAME SIZE and MAX RATE be an input parameters although implementation specific ones? I would also note that they are not written as parameters or constants here for any programming language I know of. 4. Section 4.2.1 // if (bckt_id->t_exp risks overflow) // Details not shown // return EXIT_SANCTION; I don't understand this. If the issue that one attempts to protect against is that t_exp becomes larger than what the variable supports? What I can see of how t_exp is used it must not wrap and is carrying the devices understanding of time progression in T_RES ticks. So what overflow is intended here? And I would note that fill_bucket prevents t_exp from never being more that qLSCORE_MAX further into the future than now. 5. Section 4.2.2: What happens when ATTEMPTS * BI_SIZE > 32? The lack of handling of this case, is it assumed that one would never configure this algorithm so that (ATTEMPTS - N)* BI_SIZE > 32 would be true for N=2 or larger where h32 would be zeros and empty for multiple attempts. It appears like there will be no significant failure, only hash collisions which reduces the actual attempts to fewer than ATTEMPTS. Should there be a warning here that if (ATTEMPTS - 1) * BI_SIZE is larger than 32 bit a larger hash size should be used? 6. Section 5.5: This might be detected when the bucket expiry time t_exp exceeds a threshold, which could also conveniently prevent overflow of the t_exp variable (see the qprotect() function in Section 4.2.1). In relation to earlier comment. It is not clear what is overload here.