QUIC ACK Manager, Statistics Manager and Congestion Control API#18676
QUIC ACK Manager, Statistics Manager and Congestion Control API#18676hlandau wants to merge 11 commits intoopenssl:masterfrom
Conversation
hlandau
added
branch: master
hlandau
force-pushed
the
quic-loss-detector-final
branch
3 times, most recently
from
f23b13d to
8383bf8
Compare
|
Updated, minor changes. Rebased. @paulidale |
hlandau
force-pushed
the
quic-loss-detector-final
branch
from
9d2884e to
a00dc9c
Compare
hlandau
force-pushed
the
quic-loss-detector-final
branch
from
a00dc9c to
da292d6
Compare
|
Rebased on top of frame encoding PR. Reviews invited. |
github-actions
bot
added
the
severity: fips change
| /* Callback called if frames in this packet are lost. arg is cb_arg. */ | ||
| void (*on_lost)(void *arg); | ||
| /* Callback called if frames in this packet are acked. arg is cb_arg. */ | ||
| void (*on_acked)(void *arg); | ||
| /* | ||
| * Callback called if frames in this packet are neither acked nor lost. arg | ||
| * is cb_arg. | ||
| */ | ||
| void (*on_discarded)(void *arg); | ||
| void *cb_arg; |
There was a problem hiding this comment.
What is the reason to have per-packet callbacks?
There was a problem hiding this comment.
There may be no particular need. We could have the function pointer be set when the ACKM is created but cb_arg still kept here. Though it seems a little unusual to separate the two.
There was a problem hiding this comment.
It would save the 3 pointers, setting them on each packet, etc. Also should the callback have a pointer to the actual packet as an argument?
There was a problem hiding this comment.
Another possibility is to have one callback with an int outcome parameter (LOST, ACKED or DISCARDED), which would probably make more sense
| */ | ||
| struct pn_set_item_st { | ||
| struct pn_set_item_st *prev, *next; | ||
| OSSL_QUIC_ACK_RANGE range; | ||
| }; | ||
|
|
||
| struct pn_set_st { | ||
| struct pn_set_item_st *head, *tail; | ||
|
|
||
| /* Number of ranges (not PNs) in the set. */ | ||
| size_t num_ranges; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Perfect analysis!
Food for thought. Could we use a BIGNUM as a bit set instead to represent the PNs in the set? As we remove only by raising the watermark and removing ranges below the watermark - this could be a simple BN_shift(). We insert PN by PN - so this is just a BN_set_bit() operation. The most expensive operation would then be the conversion of the bits set to OSSL_QUIC_ACK_RANGEs to be sent in ACK frames. I still think it should not be too expensive.
There was a problem hiding this comment.
Interesting possibility. Interested to hear @paulidale's thoughts
|
I'll continue reviewing this tomorrow. |
t8m
left a comment
t8m
left a comment
There was a problem hiding this comment.
This is great work! I did not finish reviewing yet.
|
Rebased and updated. |
hlandau
force-pushed
the
quic-loss-detector-final
branch
from
da292d6 to
e449fbe
Compare
github-actions
bot
removed
the
severity: fips change
|
Great work! |
mattcaswell
left a comment
mattcaswell
left a comment
There was a problem hiding this comment.
I'm still going through this. But here are some comments so far.
| */ | ||
| unsigned int pkt_space :2; | ||
|
|
||
| /* 1 if the packet is in flight. */ |
There was a problem hiding this comment.
I'm not sure what this means in this context. Won't this always be set if we are calling ossl_ack_on_tx_packet - because we just transmitted the packet.
There was a problem hiding this comment.
This is straight from the RFC pseudocode, curiously. I'm not actually sure what it's for and can't find it modelled in the other implementations I've looked at. So we may be able to remove it.
hlandau
force-pushed
the
quic-loss-detector-final
branch
from
7aad00a to
4bc6e51
Compare
|
Rebased on top of @paulidale's OSSL_TIME changes and updated accordingly. |
levitte
added
approval: done
openssl-machine
removed
the
approval: done
|
This pull request is ready to merge |
openssl-machine
added
the
approval: ready to merge
|
Merged to master. |
This is the initial implementation of the ACK Manager for OpenSSL's QUIC support, with supporting design documentation and tests. Because the ACK Manager also depends on the Statistics Manager, it is also implemented here. The Statistics Manager is quite simple, so this does not amount to a large amount of extra code. Because the ACK Manager depends on a congestion controller, it adds a no-op congestion controller, which uses the previously workshopped congestion control API. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #18676)
Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #18676)
This is the initial implementation of the ACK Manager for OpenSSL's QUIC support, with supporting design documentation and tests. Because the ACK Manager also depends on the Statistics Manager, it is also implemented here. The Statistics Manager is quite simple, so this does not amount to a large amount of extra code. Because the ACK Manager depends on a congestion controller, it adds a no-op congestion controller, which uses the previously workshopped congestion control API. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#18676)
Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#18676)
This is the initial implementation of the ACK Manager for OpenSSL's QUIC support, with supporting design documentation and tests. Because the ACK Manager also depends on the Statistics Manager, it is also implemented here. The Statistics Manager is quite simple, so this does not amount to a large amount of extra code. Because the ACK Manager depends on a congestion controller, it adds a no-op congestion controller, which uses the previously workshopped congestion control API. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#18676)
Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#18676)
6 participants
🎉
This is the initial implementation of the ACK Manager for OpenSSL's QUIC support, with supporting design documentation and tests.
Because the ACK Manager also depends on the Statistics Manager, it is also implemented here. The Statistics Manager is quite simple, so this does not amount to a large amount of extra code.
Because the ACK Manager depends on a congestion controller, it adds a no-op congestion controller, which uses the previously workshopped congestion control API.