Summary:
This diff hooks the DSR write function into QuicServerTransport's
write path.
Reviewed By: mjoras
Differential Revision: D27890711
fbshipit-source-id: ac4452373c0baafe091f93fb54fccf87be604a9c
Summary: For now let it fallback to ping.
Reviewed By: mjoras
Differential Revision: D27481741
fbshipit-source-id: 874529a06ab882d9651e06f615bc82505c1c2872
Summary:
The cloning scheduler will later bypass all D6D probes anyway. So the
hasData() API should answer False if only thing outstanding is D6D packets.
Reading the code, it's actually not very clear to me if the counter is correct
though. :/ So i left some TODO warning comments to the d6d enabling flag
(Note: this ignores all push blocking failures!)
Reviewed By: mjoras
Differential Revision: D27480434
fbshipit-source-id: 12ddded1b496b92ff5c03125c5283b8c7f3fcdb3
Summary:
Two bugs introduced in the pervious diff that merged write buffer and
loss buffer of a QUIC stream:
(1) We have stopped writing streams' loss buffers if connection is flow control
blocked.
(2) When stream write fails due to running out of packet space, it would be
mistakenly categorized as flow control has ran out. As a result, the loop in
writeStreamsHelper() would continue to try to write loss buffers data, assuming
loss buffer is still OK to write since they don't limit by flow control window.
But because the actually problem is the packet is full, the loss buffer write
would fail anyway. So we wasted some CPU trying to do a write that's destined
to fail.
Reviewed By: mjoras
Differential Revision: D27117026
fbshipit-source-id: db508fa101e04fbe4b5fe21c0c7013b0c0b07936
Summary: they need to be prioritized together
Reviewed By: mjoras
Differential Revision: D26918282
fbshipit-source-id: 061a6135fd7d31280dc4897b00a17371044cee60
Summary: Sending a potentially over-sized d6d packet on PTO might be a bad idea. Plus, a PTO probe containing app data sounds more useful than only "empty" packet.
Reviewed By: yangchi
Differential Revision: D25109633
fbshipit-source-id: ded0c02397b7dc4c346f3aa9b61869836791baec
Summary: As in title. No reason to make a string copy, these always refer to string literals.
Reviewed By: yangchi
Differential Revision: D25288518
fbshipit-source-id: 489f0ac22aa86aa4a4acb562245e98b6d33a10fd
Summary: Adds a top level API to set stream priorities, mirroring what is currently proposed in httpbis. For now, default all streams to the highest urgency, round-robin, which mirrors the current behavior in mvfst.
Reviewed By: mjoras
Differential Revision: D20318260
fbshipit-source-id: eec625e2ab641f7fa6266517776a2ca9798e8f89
Summary: This reduces dependencies for both testing and instrumentation.
Reviewed By: mjoras
Differential Revision: D23997313
fbshipit-source-id: 5eb3a790c7bb2569dc1e941e3911ad4aac4e9258
Summary:
Due to high number of RTT samples I refactored the OutstandingPacket to
split the packet data and packet metrics. We know have access to metrics
without the need of saving the packet data.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/178
Reviewed By: mjoras
Differential Revision: D23711641
Pulled By: bschlinker
fbshipit-source-id: 53791f1f6f6e184f37afca991a873af05909fbd2
Summary:
According to the spec v21
(https://tools.ietf.org/id/draft-ietf-tsvwg-datagram-plpmtud-21.html#name-sending-quic-probe-packets),
d6d probe should only contain a PING followed by many PADDING frames. As a first step, we fully comply with the spec.
Future optimizations:
- We can potentially put loss data in the probe packet to perform retransmission. This might make the probe packet more useful by increasing goodput.
Reviewed By: mjoras
Differential Revision: D22557962
fbshipit-source-id: 9a6584bc46aeb29981c4e2c4121ded127a7f2f06
Summary: Implemented necessary changes to emit DATA BLOCKED frame when a QUIC connection is write blocked.
Reviewed By: mjoras
Differential Revision: D23067313
fbshipit-source-id: f80d7425c9a3c4e9b81405716bcd944c83b97ac2
Summary:
It is useful to be able to account for when we detect packets as lost but actually receive an ACK for them later. This accomplishes that by retaining the packets in the outstanding packet list for a certain amount of time (1 PTO).
For all other purposes these packets are ignored.
Reviewed By: yangchi
Differential Revision: D22421662
fbshipit-source-id: 98e3a3750c79e2bcb8fcadcae5207f0c6ffc2179
Summary:
The problem with Ping being a simple frame:
(1) All SimpleFrames are in the same scheduler. So sending ping means we may
also send other frames which can be problematic if we send in Initial or
Handshake space
(2) Ping isn't retranmisttable. But other Simple frames are. So we are
certainly setting this wrong when we send pure Ping packet today.
That being said, there are cases where we need to treat Ping as retransmittable.
One is when it comes to update ack state: If peer sends us Ping, we may want to
Ack early rather than late. so it makes sense to treat Ping as retransmittable.
Another place is insertion into OutstandingPackets list. When our API user sends
Ping, then also add a Ping timeout. Without adding pure Ping packets into OP list,
we won't be able to track the acks to our Pings.
Reviewed By: mjoras
Differential Revision: D21763935
fbshipit-source-id: a04e97b50cf4dd4e3974320a4d2cc16eda48eef9
Summary:
On loss timer, currently we knock all handshake packets out of the OP
list and resend everything. This means miss RTT sampling opportunities during
handshake if loss timer fires, and given our initial loss timer is likely not a
good fit for many networks, it probably fires a lot.
This diff keeps handshake packets in the OP list, and add packet cloning
support to handshake packets so we can clone them and send as probes.
With this, the handshake alarm is finally removed. PTO will take care of all
packet number space.
The diff also fixes a bug in the CloningScheduler where we missed cipher
overhead setting. That broke a few unit tests once we started to clone
handshake packets.
The writeProbingDataToSocket API is also changed to support passing a token to
it so when we clone Initial, token is added correctly. This is because during
packet cloning, we only clone frames. Headers are fresh built.
The diff also changed the cloning behavior when there is only one outstanding
packet. Currently we clone it twice and send two packets. There is no point of
doing that. Now when loss timer fires and when there is only one outstanding
packet, we only clone once.
The PacketEvent, which was an alias of PacketNumber, is now a real type that
has both PacketNumber and PacketNumberSpace to support cloning of handshake
packets. I think in the long term we should refactor PacketNumber itself into a
real type.
Reviewed By: mjoras
Differential Revision: D19863693
fbshipit-source-id: e427bb392021445a9388c15e7ea807852ddcbd08
Summary:
The AckScheduler right now has two modes: Immediate mode which always
write acks into the current packet, pending mode which only write if
needsToSendAckImmediately is true. The FrameScheduler choose the Immdiate mode
if there are other data to write as well. Otherwise, it chooses the Pending
mode. But if there is no other data to write and needsToSendAckImmediately is
false, the FrameScheduler will end up writing nothing.
This isn't a problem today because to be on the write path, the shouldWriteData
function would make sure we either have non-ack data to write, or
needsToSendAckImmediately is true for a packet number space. But once we allow
packets in Initial and Handshake space to be cloned, we would be on the write
path when there are probe quota. The FrameScheduler's hasData function doesn't
check needsToSendAckImmediately. It will think it has data to write as long as
AckState has changed, but can ends up writing nothing with the Pending ack
mode.
I think given the write looper won't be schedule to loop when there is no
non-ack data to write and needsToSendAckImmediately is true, it's safe to
remove Pending ack mode from AckScheduler.
Reviewed By: mjoras
Differential Revision: D22044741
fbshipit-source-id: 26fcaabdd5c45c1cae12d459ee5924a30936e209
Summary: 0 is now a valid packet number, so we should make these optional. In cases where they are needed to construct packet builder, it should be safe to use 0 as default since it's only used for computing `twiceDistance` in PacketNumber.cpp.
Reviewed By: yangchi
Differential Revision: D21948454
fbshipit-source-id: af9fdc3e28ff85f1594296c4d436f24685a0acd6
Summary:
otherwise we keep encoding packet headers and potentially write into
the write buffer without actually generating a packet. It leaves a trail of bad
headers inside the buffer
Reviewed By: mjoras
Differential Revision: D21626733
fbshipit-source-id: 3bdcf6277beccb09b390a590ba2bb0eb8e68e6c1
Summary:
Becuase when we clone an existing packet, the logic inside the current
writetStreamFrameHeader is no longer correct.
Reviewed By: mjoras
Differential Revision: D21383828
fbshipit-source-id: 8e6bbb048eefd97ca7cf17b89edc2f395f274a73
Summary:
Currently the packet builder contructor will encode the packet
builder. This is fine when the builder creates its own output buffer. If later
on we decides not to use this builder, or it fails to build packet, the buffer
will be thrown away. But once the builder uses a buffer provided by caller, and
will be reused, we can no longer just throw it away if we decide not to use
this builder. So we have to delay the header encoding until we know we will use
the builder.
This is still not enough to solve the case where we want to use this builder,
it builds, then it fails . For that, we will need to retreat the tail position
of the IOBuf.
Reviewed By: mjoras
Differential Revision: D21000658
fbshipit-source-id: 4d758b3e260463b17c870618ba68bd4b898a7d4c
Summary:
Move the Initial padding code into main scheduler so that ack packets
in Initial space will also be padded.
Reviewed By: mjoras
Differential Revision: D21090338
fbshipit-source-id: dd92c2a1c4d00c58bf2470e2b070c43881a70187
Summary: It may be desirable for some applications to limit the number of streams written to a single packet, for example if they are treating streams like individual messages.
Reviewed By: yangchi
Differential Revision: D21067175
fbshipit-source-id: 75edcb1051bf7f1eb2c9b730806350f9857eabec
Summary:
Other frames have been using the BufQueue version of frame writing.
Change this for Crypto streams as well
Reviewed By: mjoras
Differential Revision: D20947921
fbshipit-source-id: 1cfa0f3806e8d74e8c8a4864498e1c06b55d5292
Summary:
Now all the schdulings are via main FrameScheduler or
CloningScheduler, this API is no longer used anywhere.
Reviewed By: mjoras
Differential Revision: D20899649
fbshipit-source-id: 89b34c6e8178a3abad10c594b24feae8308e3768
Summary:
To prepare for another configuration of this chain. With this, now we
can land all the outstanding GSO optimization diffs without having to worry
about breaking the production code.
Reviewed By: mjoras
Differential Revision: D20838453
fbshipit-source-id: 807a0c546305864e0d70f8989f31d3de3b812278
Summary:
This eliminatees some tech debt by completely removing the notion of version from the core transport parameters structure and the app token for zero rtt.
Note that for the draft-27 changes we will need to temporarily re-introduce it, but to a different layer (the extension encoding itself).
Reviewed By: JunqiWang
Differential Revision: D20073578
fbshipit-source-id: 2b55af621566bf1c20e21dd17251116de1788fa0
Summary: It makes some sense to pad initials from the server as well, as it will cause the handshake to fail earlier in scenarios where the PMTU is not symmetric.
Reviewed By: yangchi
Differential Revision: D19368832
fbshipit-source-id: fdc18af1ebe686db974df0255fb89f616b72ae29
Summary:
Previously we track them since we thought we can get some additional
RTT samples. But these are bad RTT samples since peer can delays the acking of
pure acks. Now we no longer trust such RTT samples, there is no reason to keep
tracking pure ack packets.
Reviewed By: mjoras
Differential Revision: D18946081
fbshipit-source-id: 0a92d88e709edf8475d67791ba064c3e8b7f627a
Summary:
Without this, PathChallenge would not get sent because stream data fills up
all available packets: P121111946
After the fix: P121112009
Reviewed By: JunqiWang
Differential Revision: D18298438
fbshipit-source-id: c9e268b86d6c8ce1914933cb4d0e0b76a5001a99
Summary: For protocols like HTTP/3, lacking an actual priority scheme, it's a good idea to write control streams before non control streams. Implement this in a round robin fashion in the same way we do for other streams.
Reviewed By: afrind
Differential Revision: D18236010
fbshipit-source-id: faee9af7fff7736679bfea262ac18d677a7cbf78
Summary:
The intention here was always to write to streams in a round robin fashion. However, this functionality has been effectively broken since introduction as `lastScheduledStream` was never set. We can fix this by having the `StreamFrameScheduler` set `nextScheduledStream` after it has written to the streams. Additionally we need to remove a check that kept us from moving past a stream if it still had data left to write.
In extreme cases this would cause streams to be completely starved, and ruin concurrency.
Reviewed By: siyengar
Differential Revision: D17748652
fbshipit-source-id: a3d05c54ee7eaed4d858df9d89035fe8f252c727
Summary: these names are always temporary strings that we can avoid a copy
Reviewed By: mjoras
Differential Revision: D17477801
fbshipit-source-id: b9a04e4e6df3a2ccd4a45233a7a755f87d16d570
Summary:
Make a custom variant type for PacketHeader. By not relying on boost::variant
this reduces the code size of the implementation.
This uses a combination of a union type as well as a enum type to emulate a variant
Reviewed By: yangchi
Differential Revision: D17187589
fbshipit-source-id: 00c2b9b8dd3f3e73af766d84888b13b9d867165a
Summary:
Prior to this diff we would clone out an entire flow control's worth of data from the writebuffer and then clone out a smaller portion of that to write into the packet builder. This is extremely wasteful when we have a large flow control window.
Additionally we would always write the stream data length field even when we are going to fill the remainder of the packet with the current stream frame. By first calculating the amount of data that needs to can be written and writing the header, we can now omit the data length field when we can fill the whole packet.
Reviewed By: yangchi
Differential Revision: D15769514
fbshipit-source-id: 95ac74eebcde87dd06de54405d7f69c42362e29c
Summary: These were changed to varints. To support this we need to do some extra horrible version plumbing. I don't want to keep this long term but it works for now.
Reviewed By: yangchi
Differential Revision: D16293568
fbshipit-source-id: a9ea9083be160aa3e6b338a7d70d7f00e44ec5ab
Summary:
This ensure a lot of code do not depend on fizz anymore.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/26
Reviewed By: mjoras, JunqiWang
Differential Revision: D16030663
Pulled By: yangchi
fbshipit-source-id: a3cc34905a6afb657da194e2166434425e7e163c
Summary:
There is a bug in how we decide if we should schedule a write loop due
to sending Acks. Currently if one PN space has needsToWriteAckImmediately to
true and another PN space has hasAcksToSchedule to true, but no PN space
actually has both to true, we will still schdeule a write loop. But that's
wrong. We won't be able to send anything in that case. This diff fixes that.
Reviewed By: JunqiWang
Differential Revision: D15446413
fbshipit-source-id: b7e49332dd7ac7f78fc3ea28f83dc49ccc758bb0
