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

QUIC_SEND_PACING_INTERVAL unit default to us, not need MS_TO_US() #4470

Closed
3 of 4 tasks
Bluesky202310 opened this issue Aug 17, 2024 · 1 comment · Fixed by #4543
Closed
3 of 4 tasks

QUIC_SEND_PACING_INTERVAL unit default to us, not need MS_TO_US() #4470

Bluesky202310 opened this issue Aug 17, 2024 · 1 comment · Fixed by #4543
Labels
Area: Core Related to the shared, core protocol logic Bug: Core A code bug in the Core MsQuic code external Proposed by non-MSFT
Milestone

Comments

@Bluesky202310
Copy link

Describe the bug

QUIC_SEND_PACING_INTERVAL unit default to us, MS_TO_US() expand it 1000x. so SendAllowance with no pacing will easy occur.

bbr.c
IRQL_requires_max(DISPATCH_LEVEL)
uint32_t
BbrCongestionControlGetSendAllowance(
In QUIC_CONGESTION_CONTROL* Cc,
In uint64_t TimeSinceLastSend, // microsec
In BOOLEAN TimeSinceLastSendValid
)
{
...
Bbr->MinRtt < MS_TO_US(QUIC_SEND_PACING_INTERVAL)) {
//
// We're not in the necessary state to pace.
//
SendAllowance = CongestionWindow - Bbr->BytesInFlight;
...
}

quicdef.h
//
// The number of microseconds between pacing chunks.
//
#define QUIC_SEND_PACING_INTERVAL 1000

Affected OS

  • Windows
  • Linux
  • macOS
  • Other (specify below)

Additional OS information

No response

MsQuic version

main

Steps taken to reproduce bug

just check bbr.c and quicdef.h

Expected behavior

fixed method:
bbr.c
IRQL_requires_max(DISPATCH_LEVEL)
uint32_t
BbrCongestionControlGetSendAllowance(
In QUIC_CONGESTION_CONTROL* Cc,
In uint64_t TimeSinceLastSend, // microsec
In BOOLEAN TimeSinceLastSendValid
)
{
...
Bbr->MinRtt < QUIC_SEND_PACING_INTERVAL) {
//
// We're not in the necessary state to pace.
//
SendAllowance = CongestionWindow - Bbr->BytesInFlight;
...
}

Actual outcome

SendAllowance with no pacing will easy occur.

Additional details

@nibanks
Copy link
Member

nibanks commented Aug 17, 2024

Good catch! Thank you! Feel free to make a PR to fix, too. Otherwise, we'll get to this soon hopefully.

@nibanks nibanks added Bug: Core A code bug in the Core MsQuic code external Proposed by non-MSFT Area: Core Related to the shared, core protocol logic labels Aug 17, 2024
@nibanks nibanks added this to the Future milestone Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core Related to the shared, core protocol logic Bug: Core A code bug in the Core MsQuic code external Proposed by non-MSFT
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants