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

["RFC ISE (Adrian Farrel)" <[e-mail]>]

Magnus,

Very many thanks for a helpful and clear review.

Cheers,
Adrian



Magnus Westerlund via Datatracker wrote:
> 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.
>
>
>
>


-- 
Adrian Farrel (ISE),
[e-mail]