Optimization #8939
Updated by Jeff Lucovsky about 12 hours ago
The CIDR dataset type (to be added (added in #8124) uses a per-address-family SCRWLock: one for the IPv4 radix tree, one for the IPv6 radix tree. All reader operations (dataset:isset / dataset:isnotset on the packet path) take the read lock; writers (dataset:set from packets, unix-socket dataset-add / dataset-remove / dataset-clear / dataset-add-batch) take the write lock. The write lock blocks all concurrent readers on that family until it releases. For steady-state per-entry admin updates the blocking window is a few microseconds and is invisible on the packet path. For bulk operations it is not: * dataset-clear on a large CIDR set: holds both family write locks while SCRadix{4,6}TreeRelease frees the entire tree, then SCRadix{4,6}TreeInitialize sets up empty ones. Duration scales with entry count. * dataset-add-batch: holds each family's write lock for the whole batch. A 10k-entry admin import is on the order of tens of milliseconds of lock hold on a representative box; every reader (packet worker) waits for the whole window. Even the packet-path dataset:set with attacker-controlled unique sources shows the same pattern: the microbench under tests/datasets-cidr/ documents rdlock aggregate throughput dropping by roughly 3x when a single sustained writer is added. h2. Proposal For "replace the whole set" style operations, avoid the write lock entirely on the reader side by building a new CIDRType off-thread and publishing it with a single atomic pointer swap. Old readers finish their walk on the old tree; new readers pick up the new one. The old tree is freed after a grace period. Concretely: # Change Dataset.cidr_data from a direct-owned CIDRType* to an atomic pointer. Reader path: atomic-load the pointer, take the tree's rdlock, walk, release rdlock. This adds one atomic load per lookup (negligible). # Add a bulk API DatasetReplaceCIDR(set, entries[], n): #* allocate a fresh CIDRType and populate it (no locks; nobody can see it yet). #* CAS Dataset.cidr_data to point at the new one, keeping the old pointer. #* defer-free the old tree once no worker can still be inside a walk that observed the old pointer. # Grace period detection: the cheapest options are: #* a per-worker "in-CIDR-lookup" counter that the swap thread waits to see quiesce, or #* defer-free at the next flow-manager tick or packet-pool reload sync point, whichever exists first. The former is more responsive; the latter is simpler and reuses existing synchronization. # Wire the new API into UnixSocketDatasetClear (for CIDR sets) and optionally UnixSocketDatasetAddBatch when the caller asks for "replace" semantics (add a boolean argument, or a separate dataset-replace-batch command). h2. Non-goals * Per-entry dataset:set from packets keeps using the current rdlock-first / wrlock-on-miss pattern. Shadow-swap on a per-entry basis is not the right shape for that path. The write-lock-under- spoofed-flood risk documented in datasets.rst is unaffected by this issue. * dataset:unset from packets stays on the write lock; unset is used much less than set and the per-entry cost is comparable. h2. Semantics to preserve * Concurrent packet-path dataset:set entries that arrive during a shadow-swap end up in whichever version the worker's atomic-load pointed at. Existing docs already note that "clear + batch is not atomic against concurrent packet-path writes"; that stays true and the doc note doesn't need to change. * unix-socket admin ops themselves remain serialized by the unix-command thread, so admin cannot race admin. h2. Rough scope * src/datasets.h: atomic pointer type change for cidr_data. * src/datasets-cidr.[ch]: new DatasetReplaceCIDR + per-worker quiescence hook or defer-free integration. * src/runmode-unix-socket.c: wire dataset-clear (and optionally dataset-add-batch with replace=true) to the new API for CIDR type. * doc/userguide/rules/datasets.rst: update the "schedule bulk imports during quiet windows" guidance to note that shadow-swap has displaced it for replace-style admin ops. The write-lock-under- attack note for packet-path dataset:set stays. * tests/datasets-cidr: add coverage that concurrent readers see either the old or new state through a swap, and never a torn state. h2. Related * #8124 (base CIDR dataset feature) * Only the bulk-import ("clear + reload") guidance in datasets.rst goes away when this lands. The per-packet dataset:set flood note stays -- that path uses per-entry writes and is out of scope here.