diff --git a/dc/s2n-quic-dc/src/path/secret/map/state.rs b/dc/s2n-quic-dc/src/path/secret/map/state.rs index bc7729239..3dee5e938 100644 --- a/dc/s2n-quic-dc/src/path/secret/map/state.rs +++ b/dc/s2n-quic-dc/src/path/secret/map/state.rs @@ -34,6 +34,13 @@ pub(crate) struct PeerMap( pub(crate) struct IdMap(Mutex>>); impl PeerMap { + fn reserve(&self, additional: usize) { + self.0 + .lock() + .unwrap_or_else(|e| e.into_inner()) + .reserve(additional, |e| self.hash(e)); + } + fn hash(&self, entry: &Entry) -> u64 { self.hash_key(entry.peer()) } @@ -87,6 +94,13 @@ impl PeerMap { } impl IdMap { + fn reserve(&self, additional: usize) { + self.0 + .lock() + .unwrap_or_else(|e| e.into_inner()) + .reserve(additional, |e| self.hash(e)); + } + fn hash(&self, entry: &Entry) -> u64 { self.hash_key(entry.id()) } @@ -137,24 +151,6 @@ impl IdMap { } } -// # Managing memory consumption -// -// For regular rotation with live peers, we retain at most two secrets: one derived from the most -// recent locally initiated handshake and the most recent remote initiated handshake (from our -// perspective). We guarantee that at most one handshake is ongoing for a given peer pair at a -// time, so both sides will have at least one mutually trusted entry after the handshake. If a peer -// is only acting as a client or only as a server, then one of the peer maps will always be empty. -// -// Previous entries can safely be removed after a grace period (EVICTION_TIME). EVICTION_TIME -// is only needed because a stream/datagram might be opening/sent concurrently with the new -// handshake (e.g., during regular rotation), and we don't want that to fail spuriously. -// -// We also need to manage secrets for no longer existing peers. These are peers where typically the -// underlying host has gone away and/or the address for it has changed. At 95% occupancy for the -// maximum size allowed, we will remove least recently used secrets (1% of these per minute). Usage -// is defined by access to the entry in the map. Unfortunately we lack any good way to authenticate -// a peer as *not* having credentials, especially after the peer is gone. It's possible that in the -// future information could also come from the TLS provider. pub(super) struct State where C: 'static + time::Clock + Sync + Send, @@ -247,6 +243,7 @@ where rehandshake_period: Duration::from_secs(3600 * 24), peers: Default::default(), ids: Default::default(), + eviction_queue: Default::default(), cleaner: Cleaner::new(), signer, control_socket, @@ -256,6 +253,15 @@ where request_handshake: RwLock::new(None), }; + // Growing to double our maximum inserted entries should ensure that we never grow again, see: + // https://github.com/rust-lang/hashbrown/blob/3bcb84537de01372cab2c1cd3bbfd8577a67ce05/src/raw/mod.rs#L2614 + // + // In practice we don't pin a particular version of hashbrown but there's definitely at + // most a constant factor of growth left (vs continuous upwards resizing) with any + // reasonable implementation. + state.peers.reserve(2 * state.max_capacity); + state.ids.reserve(2 * state.max_capacity); + let state = Arc::new(state); state.cleaner.spawn_thread(state.clone()); @@ -522,8 +528,8 @@ where queue.push_back(Arc::downgrade(&entry)); - // We are out of room, need to prune some entries. - if queue.len() == self.max_capacity { + // We went beyond queue limit, need to prune some entries. + if queue.len() > self.max_capacity { // FIXME: Consider a more interesting algorithm, e.g., scanning the first N entries // if the popped entry is still live to see if we can avoid dropping a live entry. // May not be worth it in practice.