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

Updated ACK_FREQUENCY according to draft-ietf-quic-ack-frequency-10 #4670

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

gaurav2699
Copy link
Contributor

@gaurav2699 gaurav2699 commented Nov 28, 2024

Description

This PR updates the ACK_FREQUENCY frame to the latest draft. A new reordering threshold is added which indicates the maximum number of packets that can be out of order before an immediate acknowledgment (ACK) is triggered.

Testing

New Unit test cases have been added to test the reordering threshold logic.

Documentation

No

@gaurav2699 gaurav2699 requested a review from a team as a code owner November 28, 2024 18:03
@gaurav2699 gaurav2699 requested review from nibanks and sourad November 28, 2024 18:05
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 91.11111% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.76%. Comparing base (9640abe) to head (a928db6).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
src/core/ack_tracker.c 88.00% 3 Missing ⚠️
src/core/connection.c 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4670      +/-   ##
==========================================
- Coverage   87.27%   85.76%   -1.52%     
==========================================
  Files          56       56              
  Lines       17361    17384      +23     
==========================================
- Hits        15152    14909     -243     
- Misses       2209     2475     +266     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/core/connection.c Outdated Show resolved Hide resolved
src/core/connection.h Show resolved Hide resolved
src/core/connection.h Show resolved Hide resolved
@nibanks
Copy link
Member

nibanks commented Dec 2, 2024

Looks like CLOG needs to be updated again.

src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/frame.h Outdated Show resolved Hide resolved
@nibanks
Copy link
Member

nibanks commented Dec 3, 2024

QUIC_TP_ID_MIN_ACK_DELAY needs to be updated to match the latest in the spec:

min_ack_delay (0xff04de1b):
A variable-length integer representing the minimum amount of time, in microseconds, that the endpoint sending this value is willing to delay an acknowledgment. This limit could be based on the data receiver's clock or timer granularity. min_ack_delay is used by the data sender to avoid requesting too small a value in the Requested Max Ack Delay field of the ACK_FREQUENCY frame.

src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.h Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.h Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Fixed Show fixed Hide fixed
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
src/core/ack_tracker.c Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants