[Tsv-art] Tsvart early review of draft-briscoe-docsis-q-protection-02

[Magnus Westerlund via Datatracker <[e-mail]>]

Reviewer: Magnus Westerlund
Review result: Ready with Issues

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.