Summary: As a drive-by: fixed a bug where I didn't cancel the pending event when timeouts expire.
Reviewed By: mjoras
Differential Revision: D23910767
fbshipit-source-id: 233e590c17a6c6b7a5f4a251bd8f78f6dfae3c0b
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:
This glues together the d6d lifecycle via probe timeout and raise timeout.
Had to put these two timeouts in the base transport because it has all the
necessary accountings (e.g. check close state, process callbacks) that should
happen before scheduling timeouts.
Other notable changes (included here because code is simple):
- Keep track of d6d probes in loss state. Upon second thought, it makes more
sense because we are reducing the available bandwidth as a result of sending
probes anyway. And not tracking them imposes a delay on congestion controller.
I think this does not violate the d6d spec's point of not penalizing congestion
window for d6d probes, because
- 1. we don't put losses of d6d probes in loss event. Therefore from the POV of
congestion controller, d6d probes never get lost.
- there will be at most kDefaultD6DMaxOutstandingProbes losses (i.e. 2)
that we don't tell congestion controller about. Even if those are actually
caused by congestion, it should have minimal impact because 2 is small and if there's really a congestion, the loss of normal packets should provide the signal.
- Pacing d6d probes
- Kick off d6d after a delay of 1s. This should filter out short-lived connections where probing is relatively expensive and less useful.
Reviewed By: mjoras
Differential Revision: D23736656
fbshipit-source-id: 8121fa8bcebab10a56a4e046c32c4e99ed6c3013
Summary:
This is the transport function that sends d6d probe to socket. It
- checks if there's the pending event `sendD6DProbePacket`, which will be set by upon d6d's probe timer expiration.
- uses the current probeSize to instantiate a D6DProbeScheduler, which will be changed by a small d6d state machine.
Reviewed By: yangchi
Differential Revision: D23193967
fbshipit-source-id: dfe6ce831cfd6ff0470e801644b95d4e8da34e87
Summary:
it turns out some client can delay ack for long time, randomly, which
may make us miss a good bandwidth update
Reviewed By: mjoras
Differential Revision: D23686348
fbshipit-source-id: 6524d08b6db03ede59a72049b4af98609740463a
Summary: Implicit ACKs, while making code a little simpler/safer, are not real ACKs. One consequence of an implicit ACK is that it potentially ACKs bytes much earlier than they would be in reality, leading to inaccurate RTT measurements and inaccurate bandwidth estimations for BBR.
Reviewed By: yangchi
Differential Revision: D23675102
fbshipit-source-id: 64f779aceffa262515fe48deb18ff571b9a8c882
Summary:
According to the spec, loss of d6d probe packet should not affect congestion
control, but AFAIU its ack should be considered a normal ack and has all the
implications of an ack (e.g. sample srtt, increment largest ack num).
Additionally, although d6d probe uses ping frame, neither sending a d6d probe
nor receiving the ack should have any bearing on either pendingEvents.sendPing
or pendingEvents.cancelPingTimeout.
To differentiate a d6d probe from a normal packet in tx/rx path, the isD6DProbe
flag is added. I also added a new struct to store d6d specific states (1 field
in this diff, more to come in subsequent diffs). In updateConnection, we
identify a d6d probe by comparing the packet sequence num with the sequence num
stored in the lastProbe field of the d6d state.
Reviewed By: mjoras
Differential Revision: D22551400
fbshipit-source-id: 85ec30c185666c3d5cf827bf03b4f92e6f22d4ec
Summary: This was probably a premature optimization and introduces complexity for dubious gain. Additionally a sequence of losses could potentially cause multiple updates to be delayed.
Reviewed By: yangchi
Differential Revision: D23628058
fbshipit-source-id: d6cf70baec8c34f0209ea791dadc724795fe0c21
Summary:
There are situations where application needs to know how many packets were transmitted for a stream on certain byte range. This diff provides *some* level of that information, by adding the `cumulativeTxedPackets` field in `StreamLike`, which stores the number of egress packets that contain new STREAM frame(s) for a stream.
~~To prevent double-counting, I added a fast set of stream ids.~~
Application could rely on other callbacks (e.g. ByteEventCallback or DeliveryCallback) to get notified, and use `getStreamTransportInfo` to get `packetsTxed` for a stream.
Reviewed By: bschlinker, mjoras
Differential Revision: D23361789
fbshipit-source-id: 6624ddcbe9cf62c628f936eda2a39d0fc2781636
Summary: we don't use it in most of our code any more
Reviewed By: sharmafb
Differential Revision: D23378968
fbshipit-source-id: 876c27e8dea0e82f939c5f1b52a1cb16bb13195d
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:
Adds support for timestamping on TX (TX byte events). This allows the application to determine when a byte that it previously wrote to the transport was put onto the wire.
Callbacks are processed within a new function `QuicTransportBase::processCallbacksAfterWriteData`, which is invoked by `writeSocketDataAndCatch`.
Reviewed By: mjoras
Differential Revision: D22008855
fbshipit-source-id: 99c1697cb74bb2387dbad231611be58f9392c99f
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: 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:
During PTO, when we try to write new data, we have been limiting to
only stream data and crypto data. This diff adds a few more types of data to
write during probes if they are available: Simple frame, Window updates, Rst
and Blocked frames.
Right now Acks won't be included here.
Reviewed By: mjoras
Differential Revision: D21460861
fbshipit-source-id: d75a296e96375690eee5e609efd7d1b5c103e8da
Summary: This is just in wrong place. There can be multiple frames per packet that we may be implicitly ACKing.
Reviewed By: lnicco
Differential Revision: D21493553
fbshipit-source-id: 89befab01c5a27d400089478717110bca8137d99
Summary: Without doing this the loss buffer can potentially carry data indefinitely.
Reviewed By: yangchi
Differential Revision: D21484470
fbshipit-source-id: 85ac9f274c9badb51eb431b85f67a654c399a018
Summary:
Now we won't have a zero PTO and we will properly clear out the outstanding packets.
Note that this cipher dropping is not what the draft prescribes, instead dropping both the initial and handshake ciphers when we know 1-rtt communication is functioning.
Reviewed By: yangchi
Differential Revision: D20388737
fbshipit-source-id: 0b89eb80c8faa796ab09eda3eaa10a00dcf7bae9
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: This is useful when you want to ensure that the IOBuf you pass in is encrypted inplace, as opposed to potentially creating a new one.
Reviewed By: yangchi
Differential Revision: D21135253
fbshipit-source-id: 89b6e718fc8da1324685c390c721a564bb77d01d
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:
As it turns out, the extra indirection from storing a unique_ptr is not worse than the gain from using an `F14ValueMap` versus an `F14VectorMap`.
This reduces the `find` cost measurably in profiles, and doesn't appear to have any real negative effects otherwise.
Reviewed By: yangchi
Differential Revision: D20923854
fbshipit-source-id: a75c4649ea3dbf0e6c89ebfe0d31d082bbdc31fd
Summary:
Currently we return the exact writable bytes number from a real
congestion controller or Path Challenger. This diff round the number up to the
nearest multiple of packet length. Doing so can greatly reduce weird bytes
counting/checking bugs we have around packet writing.
Reviewed By: mjoras
Differential Revision: D20265678
fbshipit-source-id: 2973dde3acc4b2008337127482185f34e16efb43
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:
In a recent diff this was mistakenly changed to be the earliest sent
time, which makes the loss alarm duration much shorter, then we end up firing
them a lot.
Reviewed By: mjoras
Differential Revision: D19826034
fbshipit-source-id: e190d30655533fdb220e92eddc5754fb9abd007b
Summary:
This diff:
1) introduces `EnumArray` - effectively an `std::array` indexed by an enum
2) changes loss times and `lastRetransmittablePacketSentTime` inside `LossState` to be an `EnumArray` indexed by `PacketNumberSpace`
3) makes the method `isHandshakeDone()` available for both client and server handshakes.
4) uses all those inputs to determine PTO timers in `earliestTimeAndSpace()`
Reviewed By: yangchi
Differential Revision: D19650864
fbshipit-source-id: d72e4a0cf61d2dcb76f0a7f4037c36a7c8156942
Summary: PTO sending behavior has to be changed to implement per-number-packet-space PTO. This refactoring unifies the PTO sending code in order to avoid duplicating it in both functions.
Reviewed By: yangchi
Differential Revision: D19673324
fbshipit-source-id: 99b4ef5f86039ffec2b4330a6d890c5fc8df2cfd
Summary:
The fizz `encrypt` API will opportunistically use inplace encryption if the `IOBuf` you pass it is correctly sized. If it is not it will manually create the output buffer.
It turns out doing inplace versus encrypting with a given target output is a bit faster in microbenchmarks, and it will also allow experimenting with caching `IOBuf`s on the send path.
Reviewed By: knekritz
Differential Revision: D19506291
fbshipit-source-id: 3ef41290538ceac34a344114badbd167e2c25a50
