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

[Bob Briscoe <[e-mail]>]

Magnus,

Thank you for your thorough and detailed review. I scanned it yesterday, 
but realized that I need to sit down and go through it with a clear 
head. So a response will appear in a few days.

Thanks again.

Bob

On 18/02/2022 18:59, RFC ISE (Adrian Farrel) wrote:
> 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.
>>
>>
>>
>>
>

-- 
________________________________________________________________
Bob Briscoe                               http://bobbriscoe.net/