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

From: Magnus Westerlund via Datatracker <[email-masked]>
Date: Fri, 18 Feb 2022 06:32:26 -0800

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.

---

From: "RFC ISE (Adrian Farrel)" <[email-masked]>
Date: Fri, 18 Feb 2022 18:59:35 -0000

Magnus,

Very many thanks for a helpful and clear review.

Cheers,
Adrian

-- 
Adrian Farrel (ISE),
[email-masked]

---

From: Bob Briscoe <[email-masked]>
Date: Sat, 19 Feb 2022 23:19:18 +0000

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

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

---

From: Bob Briscoe <[email-masked]>
Date: Wed, 23 Feb 2022 23:35:16 +0000

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

>>> This appears to be a calculation that is based on that NBUCKETS and 
>>> MASK
>>> are

[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|

>>> implementation specific ones? I would also note that they are not 
>>> written
>>> as

[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;

>>> 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
>>> time progression in T_RES ticks. So what overflow is intended here? 
>>> And I
>>> would

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

>>> N)* BI_SIZE > 32 would be true for N=2 or larger where h32 would be 
>>> zeros
>>> and
>>> 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

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

>>> which could also conveniently prevent overflow of the t_exp variable 
>>> (see
>>> the

[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 Briscoehttp://bobbriscoe.net/

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 28 Feb 2022 08:33:08 +0000

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 <[email-masked]> 
Sent: den 24 februari 2022 00:35
To: Magnus Westerlund <[email-masked]>; Greg White <[email-masked]>
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: Re: [Tsv-art] Tsvart early review of draft-briscoe-docsis-q-protection-02

Magnus, 

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.

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 

Magnus, 

Cheers, 
Adrian 

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. 

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: 

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:

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

2. Section 4.1: 

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: 

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

4. Section 4.2.1 

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

[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>