Summary:
Previously, we only had support for notifying observers when a QUIC socket
became application rate limited.
This change adds a similar notification when a socket does not become
application rate limited, i.e when the application *starts* writing data to the
socket - either for the first time on a newly established socket or after a
previous block of writes were written and the app had no more to write.
In addition, we include the number of outstanding packets (only those that are
carrying app data in them) so that observers can use this data to timestamp the
start and end of periods where the socket performs app data writes.
Reviewed By: yangchi
Differential Revision: D26559598
fbshipit-source-id: 0a8df7082b83e2ffad9b5addceca29cc03897243
Summary: they need to be prioritized together
Reviewed By: mjoras
Differential Revision: D26918282
fbshipit-source-id: 061a6135fd7d31280dc4897b00a17371044cee60
Summary:
Update retransmission queue in the stream after BufferMeta is written,
for both new data and lost data cases.
Reviewed By: mjoras
Differential Revision: D26260987
fbshipit-source-id: 15a4fa17826426b4b972b63cf370fe791b3101ff
Summary:
- Adds counter of the number of ack-eliciting packets sent; useful for understanding lost / retransmission numbers
- Adds the following to `OutstandingPacketMetadata`
- # of packets sent and # of ack-eliciting packets sent; this enables indexing of each packet when processed by an observer on loss / RTT event, including understanding its ordering in the flow of sent packets.
- # of packets in flight; useful during loss analysis to understand if self-induced congestion could be culprit
- Fixes the inflightBytes counter in `OutstandingPacketMetadata`; it was previously _not_ including the packets own bytes, despite saying that it was.
Right now we are passing multiple integers into the `OutstandingPacket` constructor. I've switched to passing in `LossState` to avoid passing in two more integers, although I will need to come back and clean up the existing ones.
Differential Revision: D25861702
fbshipit-source-id: e34c0edcb136bc1a2a6aeb898ecbb4cf11d0aa2c
Summary:
I think this should just work without the trailing `_E`. It was added
when we mixed up our own union based variant and boost::variant. Some compiler
flags didn't like that. Now we no longer have mixed up cases, this should be
fine
Reviewed By: lnicco
Differential Revision: D25589393
fbshipit-source-id: 6430dc20f8e81af0329d89e6990c16826da168b8
Summary:
We currently have a count of the number of packets retransmitted, and the number of packets marked as lost spuriously, but we do *not* have the total number of packets sent or lost, which is necessary to calculate statistics such as % of packets retransmitted % of losses that are spurious.
At the moment, we do not count loss events for D6D packets. Will likely add a separate counter for those in a subsequent diff.
Reviewed By: xttjsn
Differential Revision: D25540531
fbshipit-source-id: 80db729eb8c91f7805d342f4aab302a5b3ca4347
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: Otherwise the server can just discard them.
Reviewed By: yangchi
Differential Revision: D24208150
fbshipit-source-id: 75a7ab8156d1d0109538ffdbe8de34f6482f1c67
Summary:
Here are some of the questions I guess we'd like to answer regarding d6d:
- How long, and how many probes does d6d take to find the PMTU upper bound for a
given connection?
- If PMTU blackhole is detected, what's the packet size that triggers the
signal? And what's the state of d6d when that happens? Also, if there's issue
with the PMTU stability in the path, in what frequency does it oscillate?
This adds some meta data in d6d's lifecycle in order to provide info when those
events happen.
Reviewed By: mjoras
Differential Revision: D23972069
fbshipit-source-id: f6a2d1d656b33d94b41ecfbb0c06bdaf299caa8b
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
