Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify ECN counters #1913

Merged
merged 2 commits into from Oct 29, 2018
Merged

Clarify ECN counters #1913

merged 2 commits into from Oct 29, 2018

Conversation

martinthomson
Copy link
Member

This deals with the distinction between IP and QUIC packets better,
clarifies the interaction with packet coalescing, and makes it clear
that counts are ACK state and therefore separated by packet number
space.

Closes #1838.

This deals with the distinction between IP and QUIC packets better,
clarifies the interaction with packet coalescing, and makes it clear
that counts are ACK state and therefore separated by packet number
space.

Closes #1838.
@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Oct 25, 2018
ECN counters. For example, if one each of an Initial, 0-RTT, Handshake, and
1-RTT QUIC packet are coalesced, the corresponding counters for the Initial and
Handshake packet number space will be incremented by one and the counters for
the 1-RTT packet number space will be increased by two.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, well that's weird.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent with our mantra that coalesced packets are treated just as if each was received separately, though.

Copy link
Contributor

@gloinul gloinul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also should change these cases:

Section (ECN Counters)
On receiving a packet with an ECT or CE codepoint, an endpoint that can access the IP ECN codepoints increases the corresponding ECT(0), ECT(1), or CE count, and includes these counters in subsequent (see {{processing-and-ack}}) ACK frames (see {{frame-ack}}).

This needs rewriting a bit, to make clear that you are acking the QUICK packet but taking the ECN codepoint from the IP packet.

Section (ECN Verification)
I wonder if we have the potential of conflict on ECN codepoints to set when coalesing packets. I think the main issue is if the Handshake number space determines that ECN is not working, and then a 1-RTT packet has been generated but not sent yet, that made the assumption that the outgoing IP header shall be ECT.

"If a packet sent with an ECT codepoint is newly acknowledged by the peer in an ACK frame without ECN feedback, the endpoint stops setting ECT codepoints in subsequent packets, with the expectation that either the network or the peer no longer supports ECN."

The above sentence also need to clarify the relation between IP packets and QUIC packets.

"If verification fails, then the endpoint ceases setting ECT codepoints in subsequent packets with the expectation that either the network or the peer does not support ECN."

in subsequent IP packets.

In ACK Frame Section:

"If the frame type is 0x1b, ACK frames also contain the sum of ECN marks received on the connection up until this point."

... sum of QUIC packets with associated ECN marks received ...

In ACK block section:

The ACK frame uses the least significant bit(bit (that is, type 0x1b) to indicate ECN feedback and report receipt of packets with ECN codepoints of ECT(0), ECT(1), or CE in the packet's IP header.

This probably have no issues, but to be consisten this should be changed:

The ACK frame uses the least significant bit(bit (that is, type 0x1b) to indicate ECN feedback and report receipt of QUIC packets with ECN codepoints of ECT(0), ECT(1), or CE in the packet's IP header.

In ACK QUIC section:
Make it explicit that it is QUIC packets that the counter counts.

{{processing-and-ack}} with an ACK frame. If an endpoint does not have access
to received ECN codepoints, it acknowledges received packets per
{{processing-and-ack}} with an ACK frame.
If an endpoint receives a QUIC packet without an ECT or CE codepoint, it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should say: If an endpoint receives a QUIC packet without an associated ECT or CE codepoint from the IP header, it ...


Coalesced packets (see {{packet-coalesce}}) mean that several packets can share
the same IP header. ECN counters count each QUIC packet, not the enclosing IP
packet or UDP datagram.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the second sentence is a bit lacking in clarity. Maybe more like: The ECN counter for the ECN codepoint received in the associated IP header are incremented once for each QUIC packet, not per enclosing IP packet or UDP datagram.

@martinthomson
Copy link
Member Author

Thanks for reviewing @gloinul,

I wonder if we have the potential of conflict on ECN codepoints to set when coalesing packets. I think the main issue is if the Handshake number space determines that ECN is not working, and then a 1-RTT packet has been generated but not sent yet, that made the assumption that the outgoing IP header shall be ECT.

Yes, this is a tricky one, but I think at some point we have to leave this sort of thing to implementations. If you are coalescing and you have concluded differently for Handshake and 1-RTT, then you have to make a hard call. An endpoint that receives mixed signals about bleaching might be better off assuming that there is bleaching on the path. I think that we already have text that supports that conclusion.

BTW, for IP-layer fragmentation and reassembly where fragments come in with different markings, I am assuming here that there are rules about reassembly elsewhere that deal with this case and we don't need to concern ourselves with that in this document.

@gloinul
Copy link
Contributor

gloinul commented Oct 29, 2018

I reviewed the latest changes they look fine.

Yes, this is a tricky one, but I think at some point we have to leave this sort of thing to implementations. If you are coalescing and you have concluded differently for Handshake and 1-RTT, then you have to make a hard call. An endpoint that receives mixed signals about bleaching might be better off assuming that there is bleaching on the path. I think that we already have text that supports that conclusion.

Yes, I am fine with leaving this to implementation.

BTW, for IP-layer fragmentation and reassembly where fragments come in with different markings, I am assuming here that there are rules about reassembly elsewhere that deal with this case and we don't need to concern ourselves with that in this document.

RFC 3168 specifies that:
5.3. Fragmentation

ECN-capable packets MAY have the DF (Don't Fragment) bit set.
Reassembly of a fragmented packet MUST NOT lose indications of
congestion. In other words, if any fragment of an IP packet to be
reassembled has the CE codepoint set, then one of two actions MUST be
taken:

  * Set the CE codepoint on the reassembled packet.  However, this
    MUST NOT occur if any of the other fragments contributing to
    this reassembly carries the Not-ECT codepoint.

  * The packet is dropped, instead of being reassembled, for any
    other reason.

If both actions are applicable, either MAY be chosen. Reassembly of
a fragmented packet MUST NOT change the ECN codepoint when all of the
fragments carry the same codepoint.

@martinthomson
Copy link
Member Author

Thanks Magnus!

@martinthomson martinthomson merged commit a9d071e into master Oct 29, 2018
@martinthomson martinthomson deleted the ecn-counting-cleanup branch October 29, 2018 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants