Subject: [tcpm] draft-ietf-tcpm-accurate-ecn-09 review

From: "Ilpo Järvinen" <[email-masked]>
Date: Wed, 12 Feb 2020 16:30:11 +0200 (EET)

Hi,

Here is a list of comments/suggestions for the Accurate ECN draft.

In general

I find it odd that the draft only addresses ACK reordering in Appendix
as it's very important for the robustness of the counters (ensuring
they are never decreasing).

I think it would be helpful from implementer point-of-view to have
a table about all detectable failure conditions and how to respond to 
each.

Technical challenges and issues

CEP init

CEP initialization seems problematic as it varies depending on the
feedback but if there are more than one value that was fed back,
the endpoints do not know what the other end received. Initializing
to the same value always is one possibility, otherwise the end point
needs to keep track if it sent and CE feedback and act according to
that, the other end will catch up once ACE feedback starts. Even more
complex approach would count the number of CEs but I'm not sure if
there would be any advantage from that (and the initial value could
overflow right at the start).

Change-triggered ACKs

A change-triggered ACK is not enough for the other end to reconstruct
the ECN sequence because AccECN option in that particular ACK will
(almost always) update two counters by-definition. Therefore, the
AccECN option should be included into the change-triggered ACK and
into _the next ACK_ after that. The latter will unambiguously indicate
which of the byte counters is currently increasing.

I'd also put AccECN option into every segment that changes ACE
field to better allow using CEB to detect / rule out ACE overflows
(the algorithm given in Appendix). Estimation hopefully get it
right but ceb is most crucial to receive when there is congestion
(and the TCP header will be changing anyway in this case because
of the ACE field update).

24-bit counter updates

If an endpoint estimates the counter, unsigned delta update cannot
backtrack a counter when the estimation got it wrong (due to ACK
loss or ACK reordering). The delta should be treated as 24-bit
signed value to allow corrections to down direction.

A long run of ECN field may overflow the byte counter if the
AccECN option is not sent often enough. The option should sent like every 
2^22 received as then the currently increasing counter cannot overflow 
before the option gets scheduled again.

Mostly editorial things

Sect 2.2

"The LSBs of each of the three byte counters are carried in
the AccECN Option."

I found the double "of" structure somewhat hard to read.
Also, the use of "LSBs" here is somewhat problematic because
it was slightly earlier defined as "(three) least significant
bits" and here nothing indicates that now it is 24 bits with
the option.

Sect 2.3
cyling -> cycling

  "The Data
   Receiver is required not to delay sending an ACK to such an extent
   that the ACE field would cycle."
...there's a "Yoda English" tone in the "required not" which
is likely opposite of what you intend to say here.

Sect 3.1.1.

"The following exceptional cases need some explanation:

ECN Nonce: ..."

The "exceptional cases" sounds as it would somehow relate to the
cases in the table 2 (but it does not). To further confusion,
"ECN Nonce" really appears in the table too. At minimum, it would
be useful the swap the "ECN Nonce" and "Simul. open" cases to break
the strong ECN Nonce connection. But perhaps also the introductory
wording could be better too to distance it from the table 2 cases.

Sect 3.1.2.

   "An AccECN host (client or server) MUST ignore the three remaining
   reserved TCP header flags on all packets."
   
Is this appropriate to do, as now non-AccECN related RFCs claiming
those bits would have to update this RFC, AFAICT?

Sect 3.2.2.

Table 3 does not show how to set the client should set the ACE field,
despite the preceeding paragraph clearly assuming it does. Either
Table 3 needs to be adjusted or another table added which just contains
a straightforward SYN/ACK ECN field value -> reflector bits mapping.

Sect 5.

Doesn't mention reordering at all?

      "to ensure that any blocking of anomalous values is
      always at least under reversible policy control."
...I failed to understand what this fragment tried to say.

Appendix A.1.

   "On the arrival of an AccECN Option, the Data Sender uses the TCP
   acknowledgement number and any SACK options to calculate newlyAckedB,"
SACK required?

   "If
   newlyAckedB is negative it means that a more up to date ACK has
   already been processed, so this ACK has been superseded and the Data
   Sender has to ignore the AccECN Option."
By definition, "newlyAckedB" cannot be negative!

   "if (newlyAckedB >= 0) {"
newlyAckedB == 0 is problematic when there's ACK reordering? There's better
condition in other algorithm which takes advantage of timestamps (when)
present later in the Appendices.

   "d.ceb = (ECEB + DIVOPT - (s.ceb % DIVOPT)) % DIVOPT"
"+ DIVOPT" and the first "% DIVOPT" look superflucious to me given
modular arithmetic is being used here.

Appendix A.2.2.

   "s = d.ceb/d.cep"
Leads to divide by zero (both are defined as unsigned
integers). There should be a special case to handle the zero d.cep
correctly rather than assuming there suddenly are floats with infinity
representation in use in running code (in the later example,
"s=infinite" is assumed to be calculatable w/o an DBZ exception).

In the illustration, MSS/2 in Y-axis is really MSS/SAFETY_FACTOR,
should it be mentioned somehow?

I'd drop the number grouping commas from small values "1,460" and
"10,200", etc (it's also used inconsistently). The ~33M values
(in A.1) is probably the only one where the commas add some value
for the reader.

Appendix A.3.

    "However it would be simpler to
    merely maintain a counter packets_in_flight for the number of packets
    in flight (including control packets), which it could update once per
    RTT."
I'm not convinced that this is very simple either. Given that flightsize
is maintained more frequently than once per RTT which likely leads to some
tricky corner cases when their values are out of sync.

Appendix A.5.

2nd para, last sentence: "transmissions" -> "retransmissions"?

-- 
 i.

---

From: Bob Briscoe <[email-masked]>
Date: Fri, 14 Feb 2020 19:26:52 +0000

Ilpo, (and Mirja, there's some questions directed at you (or Richard) 
near the end),

Thank you for this careful in-depth review. Inline...

> a table about all detectable failure conditions and how to respond to
[BB] That would be useful. Given I believe you've now implemented the 
full spec, and come up with numerous corner cases, would you be willing 
to help to produce this table?
> overflow right at the start).
[BB] I think I might have already mentioned on this list that from 
draft-10 onwards we (the co-authors) already wanted to change to require 
that this counter does always initialize to one value. This was 
primarily motivated by thinking about how to introduce future extensions 
by using different initial values. We then realized we shouldn't 
unnecessarily burn more than one value from the start. Also, this info 
was already delivered reliably during the 3WHS, so incrementing the ACE 
counter was redundant (and we had to check and handle the possibility 
that the two redundant values were inconsistent).

I have already written the text for this change in our candidate for 
draft-10. I have also rationalized the draft, to bring together 
initialization of sender and receiver counters into one section 
(previously the section was only for sender counters, and initialization 
of receiver counters was elsewhere).

> of the ACE field update).
[BB] Both these suggestions sound reasonable, altho they will lead to 
more frequent AccECN Options (for those implementations that choose to 
use options). I'll write these into draft-10 unless anyone objects.
> signed value to allow corrections to down direction.
[BB] That seems easy to fix. However, I can't see that the appendix 
states that the delta is unsigned. If you're talking about Appx A.1, it 
only says the counter is unsigned. It doesn't mention what the delta is. 
Or are you concerned about this sentence and the pseudocode under it:

    Otherwise, the Data Sender
    calculates the minimum difference d.ceb between the ECEB field and
    its local s.ceb counter, using modulo arithmetic as follows:

which doesn't mention that d.ceb is unsigned. Perhaps you are concerned 
about ambiguity, given different programming languages define modulo 
arithmetic with signed types differently?

> before the option gets scheduled again.
'[BB] Again, this seems like a simple fix. To be clear, are you saying 
this should be added to the list of rules in 3.2.8 for when a Data 
Receiver includes an Option on an ACK?

> bits" and here nothing indicates that now it is 24 bits with
[BB] OK. Changed to "The 24 LSBs of each byte counter are carried in the 
AccECN Option"
To be frank, I think "The 24 LSBs of each of the three byte counter are 
carried in the AccECN Option" would be better, but if you (as a 
non-native English speaker) tripped over it, I'm not going to argue.

> is likely opposite of what you intend to say here.
[BB] Changed to "The Data Receiver is not allowed to delay sending an 
ACK to such an extent that the ACE field would cycle."

> cases in the table 2 (but it does not). To further confusion,using
[BB] OK. I've moved this Nonce wording to the end of the words about 
block 2.
And I've introduced the Simul. open rule with "The following additional 
rule does not fit the structure of the table, but complements it:"

> those bits would have to update this RFC, AFAICT?
[BB] Reworded non-normatively:

    For the avoidance of doubt, the behaviour described in the present
    specification applies whether or not the three remaining reserved
    TCP header flags are zero.

> a straightforward SYN/ACK ECN field value -> reflector bits mapping.
[BB] Good point. I've kept the table (which is for reading the field and 
only for writing it by implication). But I've changed the first sentence to:

    There is only one exception to this rule: On the final ACK of the
    3-way handshake (3WHS), a TCP client (A) in AccECN mode MUST feed
    back which of the 4 possible values of the IP-ECN field were on the
    SYN/ACK using the same binary encoding as that used on the SYN/ACK
    (see Table 2).

    Table 3 shows the meaning of each possible value...

Do you think this reference to Table 2 is clear enough? Or do you think 
we ought to produce a small 4x2 table repeating the relevant 4 codes?

> Doesn't mention reordering at all?
[BB] Added the following at the end of the Resilience bullet:

       And if data or
       ACKs are reordered, stale congestion information can be identified
       and ignored.

And at the end of the bullet listing 3 ways in which AccECN is not 
resilient to reordering:

       Following ACK reordering, the Data Sender can
       reconstruct the order in which feedback was sent, but not until
       all the missing feedback has arrived.

Did you have anything else specific in mind?

> ...I failed to understand what this fragment tried to say.
[BB] OK, I admit that's rather impenetrable wording. How about:

       Then, the designers of security devices can
       understand which currently unused values might appear in future.
       So, even if they choose to treat such values as anomalous while
       they are not widely used, any blocking will at least be under
       policy control not hard-coded.  Then, if previously unused values
       start to appear on the Internet (or in standards), such policies
       could be quickly reversed.

>     acknowledgement number and any SACK options to calculate newlyAckedB,"
[BB] Surely the words 'and any' makes it clear that SACK is not 
required. Do you mean that you believe this is impossible without SACK?

> By definition, "newlyAckedB" cannot be negative!
[BB] OK, this block of text is used twice. I agree it's confusing.

Would "if there are no newly ACKd octets" be acceptable? Then in the 
pseudocode,
     "if (newlyAckedB){"

This doesn't include the ACK reordering case (see next).
> present later in the Appendices.
[BB] I'm not sure what you want me to do here.
Note also that we need to say something for the case when timestamps are 
not available.

> modular arithmetic is being used here.
[BB] With many compilers (e.g. C99), the value of d.ceb using the 
example decimal values given just below that statement (s.ceb = 
33,554,433; ECEB = 1461, DIVOPT = 2^24) would be non-positive without 
the first "% DIVOPT", but non-negative with it.

The first % DIVOPT ensures that the value in the inner parenthesis is 
<=DIVOPT so the result of the subtraction is non-negative, which is what 
you want, in case the field wraps.

Incidentally, once you can rely on d.ceb being non-negative, you can 
produce optimized code like this:
     #define DIV_MASK DIVOPT-1
     d.ceb = (ECEB + DIVOPT - (s.ceb & DIVOPT_MASK)) & DIVOPT_MASK
I'm pretty sure a compiler would not be able to do this for you, 'cos it 
would have to prove to itself that the subtraction is always 
non-negative, which would be a rather clever compiler.

While researching this answer, I've discovered that the draft ought to 
define % as the remainder operator, not the modulo operator. I learned 
that, with a remainder operator the sign of the result of a%b is the 
same as the sign of 'a'. While with a modulo operator the sign of the 
result is the same as 'b'.

> "s=infinite" is assumed to be calculatable w/o an DBZ exception).
[BB] The purpose of this pseudocode was to explain, because s represents 
the apparent prevailing segment size, which is shown in the explanatory 
plot a little later. I've added comments to tell implementers how to 
write their code:

        s = d.ceb/d.cep        % The next line alone would avoid DBZ
        if (s <= MSS) {        % if (d.ceb <= MSS * d.cep) {

> should it be mentioned somehow?
[BB] I've changed it in the illustration to MSS/SAFETY_FACTOR

> (in A.1) is probably the only one where the commas add some value
[BB] Done.

> tricky corner cases when their values are out of sync.
[BB] I think you're right. This is more than a editorial nit. 
Nonetheless, I think Mirja implemented this then wrote this section. 
Nonetheless, I think you based your implementation on Olivier's which 
was based on Mirja's. So if this Estimation algo was not already in your 
code, that probably means I'm wrong.

Mirja?

> 2nd para, last sentence: "transmissions" -> "retransmissions"?
[BB] Done
[BB] Thank you v much, Ilpo, for these detailed comments.

Bob

-- 
________________________________________________________________
Bob Briscoehttp://bobbriscoe.net/

---

From: Mirja Kuehlewind <[email-masked]>
Date: Mon, 17 Feb 2020 14:00:52 +0100

Hi Bob, hi Ilpo,

Thanks for the review!!

Just quickly replying to this one:

> On 14. Feb 2020, at 20:26, Bob Briscoe <[email-masked]> wrote:
> 
>> 
>> 
>> Appendix A.3.
>> 
> [BB] I think you're right. This is more than a editorial nit. Nonetheless, I think Mirja implemented this then wrote this section. Nonetheless, I think you based your implementation on Olivier's which was based on Mirja's. So if this Estimation algo was not already in your code, that probably means I'm wrong.
> 
> Mirja?

No this was not implemented because this is only necessary for new congestion controls that need this value (like DCCP) but I didn’t went that far in my implementation.

The “simple” idea here was to just reset the counter once per RTT as usually you only need to adapt your window once per RTT anyway. I guess that would be more clear if we replace this:

S/which it could update once per RTT/which is rest one per RTT/

Mirja

---

From: Bob Briscoe <[email-masked]>
Date: Mon, 17 Feb 2020 14:55:57 +0000

Mirja,

> S/which it could update once per RTT/which is rest one per RTT/
First, adding some letters  presumed missing (being liberal in what I 
receive ;)

S/which it could update once per RTT/which is reset once per RTT/

I think the unstated problem here is that it's easy to count all packets 
into flight, but it's hard to count them all out, because control 
packets don't get ACK'd.

Resetting each RTT would certainly solve that, but it would mean you 
have to store one counter being built up now, and another to hold the 
value that the previous round reached. Also, I'm not sure a number that 
is 1 RTT out of date would be so useful for estimating bytes from 
packets, because TCP flows aren't necessarily continuous and predictable.

I guess, to Ilpo's point that flightsize is updated more often than once 
per RTT, your answer is that the avg pkt size could only be calculated 
at the time in each RTT just before packets_in_flight is reset.

I think we need to flag that this Appendix is more tentative than the 
others, given the problem is a bit arm-wavey, and the solution is too.

Bob

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

---

From: Mirja Kuehlewind <[email-masked]>
Date: Mon, 17 Feb 2020 17:00:08 +0100

Hi Bob,

See below.

> On 17. Feb 2020, at 15:55, Bob Briscoe <[email-masked]> wrote:
> 
> Mirja,
> 
> First, adding some letters  presumed missing (being liberal in what I receive ;)
> 

Believe it or not but my keyboard is somehow broken and the e and c are particularly bad (as well as the j which is why I recently signed an email with Mira instead of Mirja…) Need to fix that!

> 
> I think the unstated problem here is that it's easy to count all packets into flight, but it's hard to count them all out, because control packets don't get ACK’d.
> 
> Resetting each RTT would certainly solve that, but it would mean you have to store one counter being built up now, and another to hold the value that the previous round reached. Also, I'm not sure a number that is 1 RTT out of date would be so useful for estimating bytes from packets, because TCP flows aren't necessarily continuous and predictable.

The idea is to not have a moving counter but simply do the calculation one per RTT for the last RTT and then reset. That does make sense if you e.g. need this information to adjust your cwnd once per RTT and you align those two events.

I know updating the congestion window becomes/is also more a continuous process rather than a once-per RTT action in most implementation, however, this algorithm was supposed to provide a simple/simplified alternative.

> 
> I guess, to Ilpo's point that flightsize is updated more often than once per RTT, your answer is that the avg pkt size could only be calculated at the time in each RTT just before packets_in_flight is reset.

Yes.
> 
> I think we need to flag that this Appendix is more tentative than the others, given the problem is a bit arm-wavey, and the solution is too.

Not sure if a would say arm-wavey but yes it's only a rough estimates but it really depends what your congestion controller needs and does and already tracks.

Mirja

---

From: Bob Briscoe <[email-masked]>
Date: Mon, 17 Feb 2020 16:26:32 +0000

> Not sure if a would say arm-wavey but yes it's only a rough estimates but it really depends what your congestion controller needs and does and already tracks.
At the end of the first para, I've added:

"Some possible ways to calculate s_ave are outlined below. The precise 
details will depend on why an estimate of marked bytes is needed."

Bob

---

From: Mirja Kuehlewind <[email-masked]>
Date: Mon, 17 Feb 2020 17:36:46 +0100

That works!

> On 17. Feb 2020, at 17:26, Bob Briscoe <[email-masked]> wrote:
> 
> 
> 
> "Some possible ways to calculate s_ave are outlined below. The precise details will depend on why an estimate of marked bytes is needed."
> 
> 
> Bob
> 
>> 
>> Mirja
>> 
>> 
>> 
>>> 
>>> 
>>> Bob
>>> 
>>>> Mirja
>>>> 
>>>> 
>>>> 
>>>> 
>>> --

---

From: "Ilpo Järvinen" <[email-masked]>
Date: Wed, 19 Feb 2020 13:09:51 +0200 (EET)

On Fri, 14 Feb 2020, Bob Briscoe wrote:

> [BB] That would be useful. Given I believe you've now implemented the full
> spec, and come up with numerous corner cases, would you be willing to help
> to produce this table?

I can try to.

> [BB] That seems easy to fix. However, I can't see that the appendix states
> that the delta is unsigned. If you're talking about Appx A.1, it only says
> the counter is unsigned. It doesn't mention what the delta is. Or are you
> concerned about this sentence and the pseudocode under it:
> 

It does not mention it which is part of the problem. ...That, however, 
doesn't make it ambiguous because only one interpretation makes sense 
given that the d.cep uses the very same formula. That d.cep uses unsigned 
delta, no?

Language dependent issues is another can of worms I wouldn't even want
to consider opening here.

> '[BB] Again, this seems like a simple fix. To be clear, are you saying this
> should be added to the list of rules in 3.2.8 for when a Data Receiver
> includes an Option on an ACK?

Yes, I think that's the most appropriate place for them. Something along 
the lines of:

	      ...
              Option if r.ec0b or r.ec1b has incremented;</t>

              <t hangText="Preventing Counter Overflows:">It SHOULD, however,
              send an AccECN TCP Option at least once for every 2^22 bytes
              received to prevent overflowing the currently incrementing byte
              counter during continual repetition.
              </t>

              <t hangText="Full-Length Options Preferred:">It SHOULD always
              ...

> Do you think this reference to Table 2 is clear enough? Or do you think we
> ought to produce a small 4x2 table repeating the relevant 4 codes?

I was going to write that extra table is not necessary but it looks like 
both Table 2 and 3 are tricky to use for this purpose because it needs
to be interpreted right-to-left order to get the message (IP-ECN field is 
in the last or in 2nd column, and what to do is in the column left of 
it). And that's in addition to the problem with extra rows that are 
confusing at least in Table 3 (Table 2 could have an additional
separator between the first 4 rows and the rest though which might make
it slightly easier to comprehend which subset is relevant).

> And at the end of the bullet listing 3 ways in which AccECN is not resilient
> to reordering:
> 

Nothing specific but I'd have found it worrying if nothing would have 
been there (if I'd have no earlier AccECN bg) giving an impression that 
reordering was not even thought of and yet it seems obvious that it
will break a delta if not considered.

>       could be quickly reversed.

I can understand it now :-).

> [BB] Surely the words 'and any' makes it clear that SACK is not required. Do
> you mean that you believe this is impossible without SACK?

No, that's not my intent. ...But now my follow up comment is that the 
resulting newlyAckedB is different depending on whether SACK is enabled or 
not. Therefore, newlyAckedB is less precise without SACK and it does cause 
some ACKs not being able to update AccECN state when timestamps are not 
available (the appendix doesn't given any resolution to it nor reassures 
that it's still OK). 

>     "if (newlyAckedB) {"
> Note also that we need to say something for the case when timestamps are not
> available.

This particular fragment did not contain the timestamp related checks 
the other similar copy has. If timestamps are not enabled, then only > 0 
should be allowed although I think that the counters themselves could be 
used to prove the ACK ordering in that case (as they should be always 
increasing sans the wraps).

Perhaps it would be useful to define a helper for these check and use that 
in both places?

> [BB] With many compilers (e.g. C99), the value of d.ceb using the example
> decimal values given just below that statement (s.ceb = 33,554,433; ECEB =
> 1461, DIVOPT = 2^24) would be non-positive without the first "% DIVOPT", but
> non-negative with it.
> 
> <=DIVOPT so the result of the subtraction is non-negative, which is what you
> want, in case the field wraps.
> 
> Incidentally, once you can rely on d.ceb being non-negative, you can produce
> optimized code like this:    
> would have to prove to itself that the subtraction is always non-negative,
> which would be a rather clever compiler.

So this +DIVOPT term is there just to allow a compiler optimization??
Even if it proves that, it's still far more complicated than necessary
for a real implementation (with 2's complement). I'm actually surprised 
the reasoning because I thought the formula is like that to specify it 
"accurately" but now it turns out it looks that complex because of 
premature optimization.

For real (2's complement) implementations, it's much more straightforward 
to do:
  delta = sign_extend_from24bits((ECEB - s.ceb) & DIVOPT_MASK) 

> define % as the remainder operator, not the modulo operator. I learned that,
> with a remainder operator the sign of the result of a%b is the same as the
> sign of 'a'. While with a modulo operator the sign of the result is the same
> as 'b'.

Remainder won't work with the d.cep that needs to be unsigned and it 
uses the very same formula (with %).

> [BB] I think you're right. This is more than a editorial nit. Nonetheless, I
> think Mirja implemented this then wrote this section. Nonetheless, I think
> you based your implementation on Olivier's which was based on Mirja's. So if
> this Estimation algo was not already in your code, that probably means I'm
> wrong.

Since nothing in Linux really depends on the byte counters currently, 
this is not implemented by me. In Linux, the problem tends to be a reverse
of what A.3 tries to do because linux CC runs in packets rather than bytes 
(except for very limited subsets such as pacing where bytes come into 
play too).

-- 
 i.