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

[Magnus Westerlund <[e-mail]>]

Hi Bob,

 

I have reviewed the diff and the changes looks okay to me and address what I raised.

 

Thanks

 

Magnus Westerlund

 

From: Bob Briscoe <[e-mail]> 
Sent: den 24 februari 2022 00:35
To: Magnus Westerlund <[e-mail]>; Greg White <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]
Subject: Re: [Tsv-art] Tsvart early review of draft-briscoe-docsis-q-protection-02

 

Magnus, 
Thank you again for the review. Greg and I have now gone through it.

See [BB] inline for responses. I've also attached a diff (Greg pls also check that).
Pls let us know if you're happy or not.

(will do David Black's comments next)




On 19/02/2022 23:19, Bob Briscoe wrote:

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%. 


[BB] Yes, you've caught us being sloppy. Nonetheless, the number has considerable leeway built into it, so it doesn't need to be precise.

The default for AGING is introduced In §3 as follows:
    "AGING, which is a configured constant of the algorithm (default 2^19 B/s ~= 4.2 Mb/s)."
So we've dialled down the implied accuracy by:
    s|4.2 Mb/s|4 Mb/s|

Then in the pseudocode:
    s|// Convert units|// Convert units (approx)|

The value of AGING using the pseudocode shown is 3.9 Mb/s, which is near enough 4 Mb/s.





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? 


[BB] After the blocks defining input parameters and constants, we've added the following:

"Throughout the peudocode, most variables are integers. The only exceptions are floating point variables representing probabilities (MAX_PROB and ProbNative) and the AGING parameter. In practice, these floats would also typically be represented by integers that would be upscaled for precision. The pseudocode omits overflow checking and it would need to be made robust to non-default input parameters."

Yes, NBUCKETS and MASK are integers.  Sry, the comment against MASK will have thrown you off. We've changed it:
s|// convenient constant, filled with ones|
 |// convenient constant, with BI_SIZE LSBs set|





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. 


[BB] Sry, these were defined in other parts of the DOCSIS spec, and we used them without explaining.
Also , thx for pointing out the missing underscores in the variable names. These were copied from the DOCSIS spec. I've fixed them, and Greg has now submitted a request to correct them in the DOCSIS spec too.

Here's the diffs:

  // Input Parameters
+ MAX_RATE;          // Configured maximum sustained rate [b/s]
  QPROTECT_ON;       // Queue Protection is enabled [Default: TRUE]

+ MAX_FRAME_SIZE = 2000;  // DOCSIS-wide constant [B]
  // Minimum marking threshold of 2 MTU for slow links [ns]
  FLOOR =  2 * 8 * MAX_FRAME_SIZE * 10^9 / MAX_RATE;






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. 


[BB] Sry, these two lines should have been deleted. They were left over from when we'd left out details of qLSCORE_MAX.
Thx for spotting this.





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? 


[BB] See addition earlier of "The pseudocode omits overflow checking and it would need to be made robust to non-default input parameters."





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. 


[BB] Sry, same problem as earlier - everything after the comma deleted, 'cos this was from before we introduced qLSCORE_MAX.

Thanks for the warnings and errors from the output log of your pseudocode parser and compiler! Awesome.
I also added some missing ';'s

Cheers


Bob










 

 





-- 
________________________________________________________________
Bob Briscoe                               http://bobbriscoe.net/ <https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-b0108c2709550a3a&q=1&e=c51e3420-d3d6-4594-8345-77632b4fe895&u=http%3A%2F%2Fbobbriscoe.net%2F>