Summary: Ensure all CCA-related settings are included in setCongestionControl(), and that it's called from the knob setting the congestion controller.
Reviewed By: kvtsoy
Differential Revision: D47741830
fbshipit-source-id: ff1d2347581c61a58f2caaff8189126930bf4e04
Summary: Rename transportSetting.BBRConfig to CongestionControlConfig to prepare it for being used by other CCAs too.
Reviewed By: hanidamlaj, mjoras
Differential Revision: D47309439
fbshipit-source-id: 56d0ddc9752789709c54a4f7cc595f94c656e49e
Summary:
The pacingTimerTickInterval transport setting conflates two options: the minimum interval a pacer can use, and the resolution of the underlying timer. This means that a higher value leads to lower timer resolution and less pacing accuracy.
This change adds a separate parameter for the timer resolution to ensure that a larger pacing tick does not degrade the pacer accuracy.
Reviewed By: mjoras
Differential Revision: D46564066
fbshipit-source-id: 0d0e54077b80da03e6e6c9baaab49a4c969966b6
Summary: We don't use it, and the OSS lib hasn't been updated in a while.
Reviewed By: mjoras
Differential Revision: D46707559
fbshipit-source-id: ec102a52183a736cfb1c0241600816a837062108
Summary:
Create and use an actual wrapper around folly::EventBase.
Later an interface will be added that the wrapper will be implementing.
Reviewed By: jbeshay
Differential Revision: D45822401
fbshipit-source-id: 3b33f796c31043ec2881b753a9b60943bdf91f1d
Summary:
Right now small packets will cause us to subtract from the packet limit during write loops. This is generally okay when using just non-DSR data. For DSR though there is an issue where if we only write a single small packet from the non-DSR path (like an ACK) we will discount it from the amount of packets the DSR path can write.
The existing workaround for this is to only discount the non-DSR path write if the number of bytes written is less than half a packet's worth. This heuristic is incomplete though and doesn't consider the cases like the following:
1. Packet limit is 5
2. Normal path writes 4 packets of size 1232, 1232, 1232, 20.
3. The DSR path only gets one packet.
The reason for this is because the normal path also wrote full packets, but ended with a short packet. The account for this we just have to discount in terms of full packet's written. In the above example, the DSR path would get the chance to write two packets. Thus we would exceed the packet limit by one packet, but it would get us closer to the desired amount of data on the wire.
Reviewed By: kvtsoy
Differential Revision: D46633105
fbshipit-source-id: 00d92499ab73fc746ea5fdb6ff31e10f06b98666
Summary:
The idea here is to add a notion of time-based sampling of certain QUIC_STATS. This allows accounting to be done via consistent distributions for comparisons.
For now limit to the server, and only implement for inflight bytes, SRTT, and CCA bandwidth.
Reviewed By: jbeshay
Differential Revision: D46410903
fbshipit-source-id: a5db1ec720a0f8bf54e04d66c0d68686660e8eaa
Summary:
The idea here is to allow a way to do incremental stream priorities while switching between streams as aggressively.
Achieving this is somewhat tricky. The easiest place to track this is to make it so the iterator in QuicPriorityQueue no-op next() until a counter reaches an increment.
This is especially impacftul for DSR, where round robining per packet is almost pathologically bad both for CPU impact but also spurious losses and low bandwidth estimates.
Thoughts?
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D46268308
fbshipit-source-id: cd5b924141365f61f8a3363bc9cb38a62e5c94cf
Summary:
The write loop functions for DSR or non-DSR are segmented today. As such, so are the schedulers. Mirroring this, we also currently store the DSR and non-DSR streams in separate write queues. This makes it impossible to effectively balance between the two without potential priority inversions or starvation.
Combining them into a single queue eliminates this possibility, but is not entirely straightforward.
The main difficulty comes from the schedulers. The `StreamFrameScheduler` for non-DSR data essentially loops over the control stream queue and the normal write queue looking for the next stream to write to a given packet. When the queues are segmented things are nice and easy. When they are combined, we have to deal with the potential that the non-DSR scheduler will hit a stream with only DSR data. Simply bailing isn't quite correct, since it will just cause an empty write loop. To fix that we need check, after we are finished writing a packet, if the next scheduled stream only has DSR data. If it does, we need to ensure `hasPendingData()` returns false.
The same needs to be done in reverse for the DSR stream scheduler.
The last major compication is that we need another loop which wraps the two individual write loop functions, and calls both functions until the packet limit is exhausted or there's no more data to write. This is to handle the case where there are, for example, two active streams with the same incremental priority, and one is DSR and the other is not. In this case each write loop we want to write `packetLimit` packets, flip flopping between DSR and non DSR packets.
This kind of round robining is pathologically bad for DSR, and a future diff will experiment with changing the round robin behavior such that we write a minimum number of packets per stream before moving on to the next stream.
This change also contains some other refactors, such as eliminating `updateLossStreams` from the stream manager.
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D46249067
fbshipit-source-id: 56a37c02fef51908c1336266ed40ac6d99bd14d4
Summary: We shouldn't attempt to write DSR data if we run out of congestion control bytes. This will just cause an empty DSR write, which shouldn't have any negative impact but it causes tperf spam.
Reviewed By: jbeshay
Differential Revision: D46126190
fbshipit-source-id: 390e04c5d6182a2e0e61d63e806522d1ea410a30
Summary:
This has been hardcoded to SRTT/25 for a long time. However, especially when using DSR this might not be the most appropriate since it can start to get close to real world SRTTs.
Control it via a knob, and also add the behavior such that setting it to 0 effectively disables the time limit.
Reviewed By: jbeshay
Differential Revision: D46084438
fbshipit-source-id: 7dc619fdff1bd5c3f8654c4936200e0340ef94f2
Summary: Allows the default stream priority (which also means scheduling policy) to be set via TransportSettings.
Reviewed By: jbeshay, hanidamlaj
Differential Revision: D45881729
fbshipit-source-id: fcb72022cd6eac2f1dafc55173d5a46a72a95dbc
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
