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

units of src_port, dst_port, minimum_congestion_window #164

Closed
junhochoi opened this issue Jul 6, 2021 · 2 comments · Fixed by #288
Closed

units of src_port, dst_port, minimum_congestion_window #164

junhochoi opened this issue Jul 6, 2021 · 2 comments · Fixed by #288

Comments

@junhochoi
Copy link

junhochoi commented Jul 6, 2021

While I read the some of the event definitions, I have some questions:

https://github.com/quicwg/qlog/blob/main/draft-ietf-quic-qlog-quic-events.md#L312

    src_port?: uint32,
    dst_port?: uint32,

uint16 for udp port numbers (https://datatracker.ietf.org/doc/html/rfc768) looks good enough.

https://github.com/quicwg/qlog/blob/main/draft-ietf-quic-qlog-quic-events.md#L1106

    initial_congestion_window?:uint64, // in bytes
    minimum_congestion_window?:uint32, // in bytes // Note: this could change when max_datagram_size changes

Since both are congestion window values, using uint64 for both makes sense to me.

@junhochoi junhochoi changed the title src_port, dst_port, minimum_congestion_window units of src_port, dst_port, minimum_congestion_window Jul 6, 2021
@rmarx
Copy link
Contributor

rmarx commented Jul 9, 2021

Hello Junho,

Thanks for opening the issue.

uint16 for UDP ports seems sensible.

Not sure if we need uint64 at all for congestion windows though... I'd rather think that using uint32 for the initial cwnd would be better? (or are there systems doing +4GB bursts on new connections somewhere? :P)

@junhochoi
Copy link
Author

congestion_window is also uint64, so I just think that initial_congestion_window and minimal_congestion_window need to be a same type. In practice it will fit into 32bit (10 x mss and 2 x mss, respectively), but no need to assign a different type for them only because they are small?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants