Summary: The log for PACING_TIMER_TICK knob was referring to another knob name. This fixes it.
Reviewed By: sharmafb
Differential Revision: D45834217
fbshipit-source-id: c26c481633218dc165a65efcf029e7c636947dea
Summary: For non-DSR streams we can fill a packet that has e.g. stream limit updates and flow control with stream data. With DSR, we can't do that and thus packets that contain this data end up counting against the packet limit. This means that we often end up writing fewer packets per write loop when using DSR. Account for this by basically discounting the non-DSR packet in a write loop if it was "small".
Reviewed By: shodoco
Differential Revision: D45754930
fbshipit-source-id: ca494a4bd042de05c12a4d45e80dd5582fe12248
Summary:
Due to the fact that store DSR and non DSR streams in separate priority queues, we end up having to make a decision on which to write first.
Longer term we should merge them into a single priority queue, but in the meantime to mitigate potential starvation, randomly flip flop between which we write first in a given write loop.
Reviewed By: jbeshay, shodoco
Differential Revision: D45701230
fbshipit-source-id: 894179e457a4d6f7364767108f9290ff267eb977
Summary:
We've long seen issues where paced congestion controllers can't keep up with the sending rate at low RTTs. While we have one mitigation for this, it only works after the pacing timer has fired. This introduces a new behavior that effectively makes the write looper ACK clock some of the time.
The way it works is that when we call updateWriteLooper, if we've already missed the next pacing wakeup or it is going to fire soon (within 1ms), we cancel it and do a write in this loop. This effectively gives us multiple opportunities to write each burst, e.g. after receiving an ACK.
Reviewed By: kvtsoy
Differential Revision: D45052984
fbshipit-source-id: 591b6f5f374f4886dffa1e9be78df02d17fcc27c
Summary: - .reset() is probably fractionally less costly than the assignment operator?
Reviewed By: sharmafb
Differential Revision: D43579041
fbshipit-source-id: 4838b6c21e94197782cf56866950be1dbf65b106
Summary:
This diff changes `QuicConnectionStateBase` so that it stores a `std::weak_ptr<SocketObserverContainer>` instead of a `std::shared_ptr<SocketObserverContainer>`.
- `QuicConnectionStateBase` needs a pointer to the `SocketObserverContainer` so that loss / ACK / other processing logic can access the observer container and send the observers notifications. There may not be a `SocketObserverContainer` if the `QuicTransportBase` implementation does not support it.
- A `SocketObserverContainer` must not outlive the instance of the `QuicTransportBase` implementation that it is associated with. This is because observers are notified that the object being observed has been destroyed when the container is destroyed, and thus if the container outlives the lifetime of the transport, then the observers will think the transport is still alive when it is in fact dead.
- By storing a weak pointer to the `SocketObserverContainer` in the `QuicConnectionStateBase`, we provide access to the observer container without extending its lifetime. In parallel, because it is a managed pointer, we avoid the possibility of dereferencing a stale pointer (e.g., a pointer pointing to an object that has since been destroyed).
Reviewed By: mjoras
Differential Revision: D42856161
fbshipit-source-id: f35558a21fea91ba794adcf9b573dd48a626ea1f
Summary:
We don't need to carry these states after the handshake is confirmed, so make them pointers instead. This will facilitate adding a structure to the AckState for tracking duplicate packets.
(Note: this ignores all push blocking failures!)
Reviewed By: hanidamlaj
Differential Revision: D41626895
fbshipit-source-id: d8ac960b3672b9bb9adaaececa53a1203ec801e0
Summary:
Store timestamps/packet numbers of recently received packets in AckState.
- The maximum number of packets stored is controlled by kMaxReceivedPktsTimestampsStored.
- The packet number of entries in the deque is guarenteed to increase
monotonically because an entry is only added for a received packet
if the packet number is greater than the packet number of the last
element in the deque (e.g., entries are not added for packets that
arrive out of order relative to previously received packets).
Reviewed By: bschlinker
Differential Revision: D37799023
fbshipit-source-id: 3b6bf2ba8ea15219a87bbdc2724fe23eebe66b70
Summary:
The normal loss detection only considers the reordering threshold per packet by comparing it with the largest ACKed packet in the ACK. This is largely based on the fact that QUIC packet numbers are monotonically increasing.
However, when using DSR there is a potential for a different host to be writing the packets to the wire. When there are multiple hosts doing this, there is a degree of reordering that is expected to happen.
Thus we can change the loss detection logic to adjust the largest packet ACKed and use the largest packet ACKed for a given DSR stream when the packet is a DSR packet. This allows for the natural reordering while detecting loss within the DSR sender's timeline of sent packets.
Note that this probably doesn't catch all of the nuances that can happen with the reordering, but it is a good start.
Reviewed By: kvtsoy
Differential Revision: D40647572
fbshipit-source-id: d84c6cd8040fb8c044ddd68fb1abc049ccddfc44
Summary: This changes the experimental behavior of BBR sending ACK_FREQUENCY frames. Now instead of using the static ack eliciting threshold, early in the connection BBR will set the threshold to 2 which has been empirically shown to be a good "initial" value.
Reviewed By: jbeshay
Differential Revision: D40438721
fbshipit-source-id: 184b8025581b6152eea97f9b557b6343d980322d
Summary:
This is useful so we can run experiments that turn this feature off while we turn ACK_FREQUENCY policies on.
(Note: this ignores all push blocking failures!)
Reviewed By: yairgott
Differential Revision: D40148536
fbshipit-source-id: 84aa4a0639b43c8721db0db6885b6af6e1701bdd
Summary:
This introduces the ability for BBR to control ACK_FREQUENCY. It also introduces a knob which will let us experiment with different policies.
This code makes BBR issue a new ACK_FREQUENCY every round trip. This may be excessive, so BBR will track the last frame sent, and only send a new one if the new value is sufficiently different in terms of milliseconds.
Note that the only dynamic part of the frame currently will be the max ack delay, which is in terms of fractional RTTs. It is not clear yet how we should vary the other parameters dynamically.
Reviewed By: kvtsoy
Differential Revision: D39910417
fbshipit-source-id: 73cfdac40ca52185c33322017a893e5a02ee58e8
Summary:
As in title. Just remove the knob trigger for now as it is causing some issues with DSR.
(Note: this ignores all push blocking failures!)
Reviewed By: shodoco
Differential Revision: D39840220
fbshipit-source-id: 253e0b2c72f9926b902585df8e5a43bebc24b79b
Summary:
The current `close` observer event marks when `closeImpl` is called. However, the actual close of the socket may be delayed for some time while the socket drains. I've changed the existing event to `closeStarted` and added a new event `closing` that marks the close of the underlying `AsyncUDPSocket`.
Adding the `closeStarted` event required two other changes which are difficult to separate from this diff:
- When a transport is destroyed, `QuicTransportBase` was calling `closeImpl` and also closing the UDP socket. However, because the `folly::ObserverContainer` used to store observers is maintained in classes that derive from `QuicTransportBase`, the observers are gone by the time the UDP socket is closed in the base class destructor. Thus, the UDP socket should be closed by the derived classes in their respective destructors. This requirement is inline with the existing code: `closeImpl` is called by all derived classes in their destructors. Made this change and added `DCHECK` statements in the `QuicTransportBase` destructor to ensure that derived classes cleanup after themselves.
- Writing tests with draining enabled and disabled required being able to set the transport settings. However, all of the existing `QuicTypedTransportTest` test cases were designed to operate after the connection was accepted (for the server impls) or established (for client impls), and transport settings cannot be updated at this state. Resolving this required adding new test classes in which the accept/connect operation is delayed until requested by the test.
Reviewed By: mjoras
Differential Revision: D39249604
fbshipit-source-id: 0ebf8b719c4d3b01d4f9509cf2b9a4fc72c2e737
Summary:
- continually issuing new connection ids to peer as old connections ids are retired through RETIRE_CONN_ID frames
- add logic to parse and act on receiving RETIRE_CONN_ID frame
Reviewed By: mjoras
Differential Revision: D38443561
fbshipit-source-id: 82fb679f482fd69c7b3a3385693d2e5575e92703
Summary: Handle MAX_PACING_RATE_KNOB_SEQUENCED transport knob with which a client can suggest a limit on the pacing rate used by the server, if it has pacing enabled. This knob also include a sequence number with which the server can use to determine out-of-order frames.
Reviewed By: bschlinker
Differential Revision: D37062350
fbshipit-source-id: 981f0c9df99cf30de6c676784409a43d45f62ff2
Summary: We want to experiment with just using adaptive thresholds for reordering.
Reviewed By: kvtsoy
Differential Revision: D37190894
fbshipit-source-id: 8c388d18c65a75ce1740cd2c193f444fe9e86ace
Summary:
This is a pretty "obvious" thing to do. When a packet with a stream frame is lost, we move that data into the lossBuffer, possibly merging it with existing data.
We track lost packets for a bit to see if they are later ACKed, which indicates the loss was "spurious". When this happens we can search the loss buffer and remove the segment of data which was actually ACKed, obviating a spurious retransmission.
Reviewed By: jbeshay
Differential Revision: D35972951
fbshipit-source-id: f5d0d4beb677d89819d77a8af297db731a8d849e
Summary:
- PTOs should not be subject to congestion control limits
- Quickly recover from PTOs being writableBytesLimited by calling onPTOAlarm() as soon as we become unblocked
Reviewed By: mjoras
Differential Revision: D35480409
fbshipit-source-id: 51500db6fff17a7badefea8bda7f63141e97f746
Summary:
This diff is part of larger change to switch to using `folly::ObserverContainer` (introduced in D27062840).
This diff:
- Changes `LegacyObserver` to inherit from the new `Observer` class by adding a compatibility layer. This compatibility layer enables existing observers to continue to be supported.
- Changes generation of observer events so that they are routed through `ObserverContainer::invokeInterfaceMethod`
- Temporarily removes some of the reentrancy protection that previously existed, as it was not being applied consistently — some events had reentrancy protection, some did not. This will be reintroduced in the next diff.
- Improves some unit tests for observers during the transition process.
Differential Revision: D35268271
fbshipit-source-id: 5731c8a9aa8da8a2da1dd23d093e5f2e1a692653
Summary:
This diff is part of larger change to switch to using `folly::ObserverContainer` (introduced in D27062840).
This diff:
- Adds methods to `QuicSocket` to support adding and removing `ObserverContainer` observers. By default, `QuicSocket` implementations will not support observers as `QuicSocket::getSocketObserverContainer` returns `nullptr`, and this will cause `QuicSocket::addObserver` and `QuicSocket::removeObserver` to fail gracefully.
- Adds support in `QuicClientTransport` and `QuicServerTransport` for `ObserverContainer` observers by overriding `getSocketObserverContainer`. Each implementation creates an `ObserverContainer` on construction that is destroyed on destruction.
- Adds the `close` event to the new observer interface and adds corresponding support in `QuicTransportBase`. This allows both the old and new observers to receive the close event. The next diff will do a hard cutover of the rest of the events.
Differential Revision: D35000960
fbshipit-source-id: e75807c97f4385532bf36244591a259aa0a2f4cc
Summary:
This implements a basic keepalive timer at the transport layer.
The basic idea is to leverage the same conditions as the idle timer. The keepalive timer will fire ~15% before the idle timer and trigger a PING frame. This has the effect of keeping the connection open if the PING is acknowledged.
Reviewed By: jbeshay
Differential Revision: D34256174
fbshipit-source-id: a8cee579f5b09bdfa23bcdc7cf4c70de299de3bd
Summary: The two lists of transport knobs in two different locations keep getting out of sync. This consolidates them into one location which is QuicConstants.
Reviewed By: mjoras
Differential Revision: D34227259
fbshipit-source-id: 26b20620f4ef85f8887e7294512155060a057839
Summary: Add new connection end cb option to client/server transport
Reviewed By: mjoras
Differential Revision: D34190871
fbshipit-source-id: 375cf95bc8dbe4ab759140cfb4b33b732869953f
Summary:
Add an experimental setting in the TokenlessPacer to use an adjustable burstSize in the to compensate for delays in pacer loop timer with a limit of 5 loop intervals.
This practically restores some concept of tokens in the pacer. This should help with pacer timer delays observed in relatively high BDP links.
To help quantify how often the delays happen, the QuicStats pacer_lagged counter is updated every time the pacer is delayed by more than 10% when the connection is not application limited.
In a test configuration across a 150 Mbps link with 150 ms round-trip delay, the experimental setting achieves ~25% higher bandwidth.
Reviewed By: bschlinker, mjoras
Differential Revision: D33035430
fbshipit-source-id: 4aa02821d609facf6c830647459319d44c6fc3bd
Summary:
Maine change is `MockConnectionCallback` -> `MockConnectionSetupCallback` + `MockConnectionCallbackNew`.
Everything else is changing tests to use the two new classes.
Differential Revision: D33076321
fbshipit-source-id: a938b63ce59f07f549b3e725caad8785348db7ed
Summary:
Experiment with using adaptive thresholds for detecting packet losses through reordering or timeouts. When transportSettings.useAdaptiveLossThresholds is set to true, every time a spurious loss is detected, the thresholds will be updated to avoid marking similar packets as losses. In cases with a high degree of packet reordering, this improves bandwidth utilization significantly.
The useAdaptiveLossThresholds setting can be set using a new transport knob.
This was verified by transferring a 5 MB object across 50 Mbps link with high random packet reordering (emulated using netem)
```
+ tc -s qdisc replace dev veth7 root handle 1:0 netem rate 50Mbit limit 1250
+ tc -s qdisc add dev veth7 parent 1:0 handle 2: netem delay 15ms 5ms distribution normal limit 1250
```
Without adaptive thresholds, ~43% of sent bytes are retransmissions. Goodput is 2 Mbps.
With adaptive thresholds, ~3% of sent bytes are retransmissions. Goodput is ~17.5 Mbps.
Reviewed By: mjoras
Differential Revision: D33354584
fbshipit-source-id: 390bfb3398d499fcdf97a5cfd5654da3eeabed17
Summary:
Adds paddingModulo to QuicPacketBuilder, which pads up short header packets so that the length remaining of a packet is a multiple of paddingModulo
ie a message of length 25 and max packet size of 40 with a paddingModulo of 8 will get 7 padding frames to a total length of 32. (and a message of length 26 will get 6 padding frames to also go up to a total length of 32)
Reviewed By: mjoras
Differential Revision: D32858254
fbshipit-source-id: 99ada01108429db9f9bba1e342daf99e80385179
Summary: - logging number of bytes the server sent during the handshake for insight.
Reviewed By: mjoras
Differential Revision: D33069800
fbshipit-source-id: e7e8f25183ee30de99e2971bae3c6b93882f6e63
Summary:
Add experimental CUBIC's hystart delay bounds that ensure CUBIC does exit Hystart too early. Current limits of 2,8 us are too short.
On a 100 Mbps link with 150 ms round-trip delay, this improved CUBIC's throughput from ~14 Mbps to ~80 Mbps.
Reviewed By: mjoras
Differential Revision: D33035403
fbshipit-source-id: bdeb081111f8053bc7fb01002316d991d8bac2e0
Summary:
This implements a global (per process) limit on unfinished handshakes from unverified source addresses.
This limits the ability of an attacker to create connection state without also allocating connection state themselves. By default the limit is 1024.
Reviewed By: kvtsoy
Differential Revision: D32772165
fbshipit-source-id: 6c195169377a9f687c54bc9782cc58fe085e1275
Summary: Otherwise we can end up in a situation where the non-DSR scheduler was limited by the loop time while the DSR scheduler is not, leading to an inconsistency.
Reviewed By: lnicco
Differential Revision: D32570027
fbshipit-source-id: f43d2517589c22bac0f2bb76626cc55c2a21fa5d
Summary:
- Change the knob param handler to receive a Transport rather than a ConnectionState.
- Add a new transport knob that can set the priority threshold and target utilization factor for automatically controlling background mode based upon active streams.
Reviewed By: mjoras
Differential Revision: D31847477
fbshipit-source-id: 4cfeff04b9fe60eb25ab484f460e252ef6970646
Summary:
Experiment with a less aggressive variant of BBR that could be used for background requests in order to improve responsiveness of foreground requests.
BBR's agressiveness can be controlled through a new transport knob CC_AGRESSIVENESS_KNOB(0xccab) which accepts values between 25-100. The value indicates the percentage of the available BW the congestion controller should try to use, leaving headroom for other flows.
Aggressiveness of BBR is reduced by adjusting BBR's startup gain during the STARTUP phase. During the ProbeBW phase, BBR uses a longer pacing gain progression with values that intentionally underutilize the available BW.
Reviewed By: bschlinker, mjoras
Differential Revision: D31219364
fbshipit-source-id: c67ffcf03d27fd4d41be2d58f00f4f960c7646c7
