Summary:
This is a simple PMTU blackhole detection method by counting packet losses
within a short window. If the cumulative packet losses exceed the threshold,
we reduce udpSendPacketLen to base pmtu. Start with a window of 5 seconds and
threshold of 5 as a first attempt.
Reviewed By: mjoras
Differential Revision: D23734194
fbshipit-source-id: 457c8204e6625bd13b366c7b0c6128e340118d57
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:
`InstrumentationObserver` is owned by `QuicSocket` now. However, to capture packet loss signal, we need it in `QuicConnectionState`. So, I refactored the code a little bit to make this easy. Changes in this code:
(a) Moved the definition of `InstrumentationObserver` and `LifeCycleObserver` to
a separate header `Observer.h`.
(b) Moved the vector of `InstrumentationObserver`s from `QuicSocket` to `QuicConnectionState`.
(c) Added a callback in `conn->pendingCallbacks` when a packet loss is detected
Reviewed By: bschlinker
Differential Revision: D23018569
fbshipit-source-id: e70d954839bdb70679ecd52d2bd1a6a6841f6778
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:
This is following a similar pattern than what was done for the client side.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/160
Reviewed By: yangchi
Differential Revision: D23560951
Pulled By: xttjsn
fbshipit-source-id: 351417cbfa3230112fff4c4de59b307f88389cf6
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:
duration_cast round towards zero. The diff uses folly::chrono::ceil
rather than std::chrono::ceil since we still need to compile with c++14
Differential Revision: D22870632
fbshipit-source-id: 18439488e879164807b1676a0105073348800412
Summary:
It seems there is a possibility of keeping a early retransmit timer
around after the packet sets it is no longer present. It's a little hard
to get to that condition though. See the test case.
Reviewed By: mjoras
Differential Revision: D22194495
fbshipit-source-id: 50cf2dcb2afe00db14c521126d338057605aa907
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:
as title. This also fixes the handshake alarm QLog. It logs before we
increased the handshake alarm count
Reviewed By: mjoras
Differential Revision: D20831207
fbshipit-source-id: c9a555884ec1c5737180c64b686793df0351a488
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 implements the handshake done signal and also cipher dropping.
Reviewed By: yangchi
Differential Revision: D19584922
fbshipit-source-id: a98bec8f1076393b051ff65a2d8aae7d572b42f5
Summary:
our convention has always been to put the mock in the test dir under
the real class
Reviewed By: lnicco
Differential Revision: D20104476
fbshipit-source-id: 5215ffc9af7a6d7a5ac41109723a71f68f852af7
Summary: no more surprises in upper layer
Reviewed By: mjoras
Differential Revision: D19976510
fbshipit-source-id: 3487e9aa2cb28d7bc748f13bc2bbc393216b4a8a
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: Some test cases used FileQLogger to verify that correct events have been logged. MockQLogger allows to check what events are logged without using an in-memory store like in FileQLogger. Migrated QuicLossFunctionsTest and QuicTransportTest from FileQLogger to MockQLOgger to simplify logic and cut dependencies.
Reviewed By: yangchi
Differential Revision: D19426423
fbshipit-source-id: eb1ae16a81656efd7c491eae790484c73dede8f3
Summary: Previously we were treating the loss buffer the same way we were the retransmission buffer. That is, each entry represented a single stream frame that was lost. There's actually no reason to do this as we treat it more like the write buffer when retransmitting. It can also lead to pathological cases where we write consecutive byte ranges as separate stream frames.
Reviewed By: yangchi
Differential Revision: D19240352
fbshipit-source-id: 94ffb397fa287688d986a4a91d531d85050d9f98
Summary:
Google shared they saw 1/4 working better than 1/8. In our own test,
we saw 10-50% retransmission reduction from this change without hurting HTTP
latency.
Reviewed By: bschlinker
Differential Revision: D19289939
fbshipit-source-id: 097cc52be4b858b242edcf52183b85391879e894
Summary:
this isn't part of the QLog draft. The ack frame information in
already included in the packet_received event. Qvis also doesn't need this.
The only additional information this gives us is that the ack frame in the
packet_received has more packets than current outstanding packets. packet_ack
only includes those still outstanding.
For example, it's possible that only packets [5, 10] are outstanding since peer
already acked [0, 4] before. But peer hasn't received the ack of ack. Then peer
sends another packet which will ack [0, 10]. In that packet_received event, the
ack frame will have [0, 10]. But we will only generate packet_ack for [5, 10].
If such information is important, then we should keep this, but make it concise
at least.
Right now i'm not even sure if such information is important. We can get to the
same conclusion by backtracking to the previously received [0, 4] ack frame.
This is a lot of data.
Thoughts?
Reviewed By: mjoras
Differential Revision: D19237301
fbshipit-source-id: 95dfe2f9f6942cc30434b1fc05dd4929607d9c95
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: `F14FastMap` is faster than `F14NodeMap` as it requires one fewer allocations. We don't need the reference stability of `F14NodeMap`. We need to make the `QuicStreamLike` movable to use it.
Reviewed By: siyengar
Differential Revision: D18681242
fbshipit-source-id: e155d0bdec905c6a2f42d7169741c130bc9cc86d
Summary:
The retransmission buffer tracks stream frame data we have sent that is currently unacked. We keep this as a sorted `deque`. This isn't so bad for performance, but we can do better if we break ourselves of the requirement that it be sorted (removing a binary search on ACK).
To do this we make the buffer a map of offset -> `StreamBuffer`.
There were two places that were dependent on the sorted nature of the list.
1. For partial reliablity we call `shrinkBuffers` to remove all unacked buffers less than an offset. For this we now have to do it with a full traversal of the retransmission buffer instead of only having to do an O(offset) search. In the future we could make this better by only lazily deleting from the retransmission buffer on ACK or packet loss.
2. We used the start of the retransmission buffer to determine if a delivery callback could be fired for a given offset. We need some new state to track this. Instead of tracking unacked buffers, we now track acked ranges using the existing `IntervalSet`. This set should be small for the typical case, as we think most ACKs will come in order and just cause existing ranges to merge.
Reviewed By: yangchi
Differential Revision: D18609467
fbshipit-source-id: 13cd2164352f1183362be9f675c1bdc686426698
Summary: The state machine logic is quite abstruse, this modifies it to make it more readable.
Reviewed By: siyengar
Differential Revision: D18488301
fbshipit-source-id: c6fd52973880931e34904713e8b147f56d0c4629
Summary:
This will allow to be able to create different kind of handshake going forward.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/59
Reviewed By: siyengar
Differential Revision: D18088574
Pulled By: mjoras
fbshipit-source-id: 0732bb63a9e243fef77cdaf4f76e711fcb09ecdc
Summary:
My goal in this stack is to properly support the functionality of issuing new
connection ids.
There are at least three cases in which this is done (server-side):
* generate initial server connection id based on host id parameters
* provide several NewConnectionId frames to the client (for use with
migration) after handshake
* responding to RetireConnectionIdFrame and issuing more NewConnectionId frames
as needed
**Changes**:
* move connIdAlgo away from parent ConnectionStateBase to just
QuicServerConnectionState
* create virtual function in ConnectionStateBase to generate
folly::none ConnectionIdData by default,
and server subclass will generate a new server id
* add unit tests to test this behaviour.
Intended for Junqi's diff of server issuing 7 new connection ids
to be rebased on this (D15178642)
Reviewed By: yangchi
Differential Revision: D17840780
fbshipit-source-id: 1a01077742d01d058eb0a726c4c9c44e39eafb24
Summary: This is just an oversight. By the time we do this things could be moved to the loss buffer if the packets were marked as lost before we got our first 1-rtt ACKs. Without this we will retransmit a lost initial indefinitely.
Reviewed By: siyengar
Differential Revision: D17989236
fbshipit-source-id: 5215bf71517f019162b307f72221877e2145e2cb
Summary:
Use the custom variant type for write frames as well, now that
we use them for read frames.
Reviewed By: mjoras
Differential Revision: D17776862
fbshipit-source-id: 47093146d0f1565c22e5393ed012c70e2e23d279
Summary:
The retransmission buffer list is sorted by offset. New written
always has higher offset than existing ones. Thus the binary search isn't
necessary.
Reviewed By: mjoras
Differential Revision: D17477354
fbshipit-source-id: d413a5c84c3831b257d5c1e6375bec56a763926b
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:
Removes clientConnId completely from ServerConnectionIdParams.
This diff first fixes an incorrect assumption; it calls
`shortHeader.getConnectionId()` which is actually the destination id (server)
not the client connection id.
Next, this entire block is unnecessary, because this will be called after
the transport is created, so the clientConnectionId will always be set.
This also setsconn.serverConnectionId earlier (shouldn't depend on
connClientId).
Reviewed By: yangchi
Differential Revision: D16792866
fbshipit-source-id: 537ba12baa9939c9d5512e46eb914c1d3a7a9aa2
Summary: Update qlog format to be more complete. Adding the summary section (with extra fields like title, description, etc). This diff is just to make the format more on par with Robin's schema.
Reviewed By: mjoras
Differential Revision: D16499808
fbshipit-source-id: 56cfbb95404f7e3c6638bffda18b53f7d83048a1
Summary:
Currently we handle crypto timer before loss timer. And currently loss
time is for AppData only since for the other two Packet Number spaces, crypto
timer will take care of it. This diff extends loss times to 3 loss times, and
handle them before handle crypto timer. The rational here is that loss time is
shorter than crypto timer, so this will make retransmission during crypto
handshake more aggressive. For app data, this diff doesn't change anything.
Reviewed By: sharma95
Differential Revision: D15552405
fbshipit-source-id: bd5c24b0622c72325ffdea36d0802d4939bae854
