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.

Sign up for GitHub

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

Jump to bottom

Fix to ECN section regarding validation #2113

Merged
merged 8 commits into from Dec 13, 2018
Merged

Fix to ECN section regarding validation #2113

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
239f4c2
Addressing issue that CE marks was not handled in per ECT type check.
gloinul Dec 11, 2018
eb7c2c8
Fixing one unclear sentence.
gloinul Dec 11, 2018
3a72fbd
Clarifies puprose of the validation bullets.
gloinul Dec 11, 2018
3faded4
Added paragraph of conditions when the comparisons should not be perf…
gloinul Dec 11, 2018
e2b6303
Apply suggestions from code review
gloinul Dec 12, 2018
0a2edd0
Removed unnecessary restriction. Added notices that local reference c…
gloinul Dec 12, 2018
43b6f67
Added text on dealing with reordered ACKs.
gloinul Dec 12, 2018
566f72e
Applying Martin Thomson's suggested fixes for restructuring.
gloinul Dec 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions draft-ietf-quic-transport.md
View file
Open in desktop
Expand Up @@ -3027,6 +3027,11 @@ frame is received:
this ACK frame. Detects if the network remark ECT(0), ECT(1) or CE to
gloinul marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this ACK frame. Detects if the network remark ECT(0), ECT(1) or CE to
this ACK frame. Detects if the network remarks ECT(0), ECT(1) or CE to

Not-ECT.

If a sender receives an ACK that contains no new acknowledgments, for example
due to reordering of the ACKs, then ECN counter comparison SHOULD NOT be
performed. Also if sender do not have state to determine if a particular PSN
gloinul marked this conversation as resolved.
Show resolved Hide resolved
is newly acknowledge or not, then the comparison SHOULD NOT be performed.
gloinul marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

The implication here seems to be that an implementation should retain extra state to distinguish between a packet that is deemed lost but not yet acknowledged and a packet that was previously acknowledged. Is that intended?

If these two cases are instead conflated, e.g. by forgetting about sent packets once either acknowledged or deemed lost, then at worst the number of newly acknowledged packets in an ACK would sometimes be under-counted, and a verification with a higher rate of false positives seems preferable to going to extra effort to decide to skip verification outright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partly, sender need to track if a packet has been acknowledged or not. But, my assumption is that the sender will have a window where such information will be discarded when likely not relevant anymore. So if everything has been ACKed up to PSN=48. Then cleaning up your state and discarding all below 48 is fine. When the reorderd ACK that indicate reception of PSN 44 and 45 can then be determined to be behind this window and ignored. The second sentence is trying to state that for this example case this is fine to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

This reads to me like the comparison of the new ECN block to the local reference (i.e. the verification procedure) should not be performed if the ACK acknowledges any packets which are outside the window and hence are not guaranteed not to be new acknowledgements. Should it instead indicate that such an acknowledgement should not be counted towards new acknowledgements?


gloinul marked this conversation as resolved.
Show resolved Hide resolved
An endpoint could miss acknowledgements for a packet when ACK frames are lost.
It is therefore possible for the total increase in ECT(0), ECT(1), and CE
counters to be greater than the number of packets acknowledged in an ACK frame.
Expand Down