delay: replace inline nanosleep with a netem-aligned real delay line (fixes #114) - #115
Open
yueguobin wants to merge 12 commits into
Open
delay: replace inline nanosleep with a netem-aligned real delay line (fixes #114)#115yueguobin wants to merge 12 commits into
yueguobin wants to merge 12 commits into
Conversation
The delay packet filter slept inline in the per-direction bridge thread (nanosleep inside delay_handler), so each direction serviced at most ~1000/latency pps. Any heavier offered load backed up in the kernel UDP socket buffer, RTT climbed without bound, and the link collapsed with packet loss — the root cause behind GNS3/gns3-server#2827. Replace it with a real delay line (new src/delay_line.[ch]): - delay_line_enqueue() copies the packet, stamps release_time = now + latency +/- jitter, and inserts into a release-ordered queue. It never blocks, so the recv loop stays drained. - a dedicated release thread per direction waits (cond_timedwait on CLOCK_MONOTONIC where available) and sends via callback when due. - delay_line_destroy() is cancel-safe (stop + broadcast + join, drops any queued packets) and runs from bridge_nios()'s pthread cleanup handler, so it is torn down on both normal exit and pthread_cancel (the real stop/delete path). bridge_nios() creates one delay line per direction when a delay filter is configured and routes sends through it; the no-filter path is unchanged. delay_handler() is now a no-op and the configured latency/jitter are read back via the new packet_filter_get_delay() accessor. pthread_cleanup_push/pop are unconditionally balanced (they expand to a do/while block on glibc, so guarding them with `if (delay_line)` would skip the whole packet loop when no delay filter is set). tests/delay/ drives the hypervisor protocol with two UDP NIOs + a delay filter and asserts: single-packet one-way ~= configured delay, a burst drains in bounded time with no loss (the old code serialized to ~N*latency), the reverse direction is bounded too, jitter stays in band, and no-filter forwarding still works. Clean under valgrind (no new leaks).
The delay line bounded latency but not memory: under sustained overload it buffered without limit (offered_rate x latency), trading the old kernel-buffer overflow for unbounded user-space growth. Add a depth limit (default 1000 packets, matching the kernel netem NETEM_LIMIT_DEFAULT) with tail-drop: once full, the newest packet is dropped so excess load is shed and memory stays bounded. Every delivered packet still transits the queue, so delivery is capped at the limit under overload. The limit is overridable without a recompile or protocol change via UBRIDGE_DELAY_LIMIT=<n>, mirroring netem's configurable limit. tests/delay: new case sets UBRIDGE_DELAY_LIMIT=20, sends 200 packets, and asserts delivery is capped (20/200) with bounded drain. Clean under valgrind with heavy tail-drop (limit=30, 500-packet bursts) -- dropped entries are freed, no new leaks.
Two things, both surfaced by actually pointing GNS3 at the build:
1. Delay filters added after bridge start now take effect. gns3-server's
add_ubridge_udp_connection() does `bridge start` THEN _ubridge_apply_filters
(reset + add on the running bridge), and re-applies on link update. The
delay line was created once at bridge_nios() start, so a delay filter
applied to a running bridge was silently inert ("configured 100ms, no
effect"). Now bridge_nios() re-reads the delay config on each packet and
lazily creates/recreates/destroys the per-direction delay line to match
(appear / params-change / disappear). The cleanup handler reads the line
through a pointer-to-pointer so pthread_cancel tears down whatever is
active even after a mid-run recreate.
2. The delay line now mirrors netem (net/sched/sch_netem.c):
- dl_tabledist() ~ tabledist() — delay = latency +/- jitter
- tfifo-style enqueue — O(1) tail append in the common
in-order case, sorted insert on jitter
reordering
- release thread ~ qdisc watchdog
(The Gaussian distribution table tc supplies to the kernel is out of scope;
this is kernel netem's dist==NULL uniform path, matching prior behaviour.)
tests/delay: new "runtime filter management" case (add/change/remove on a
running bridge) with a warmup to prime the lazy create. 15/15 PASS, stable.
Clean under valgrind across heavy runtime add/change/remove cycling.
Only delay was special-cased into the delay line; packet_loss/corrupt stay inline. Assert they still compose: a delay+loss link delivers ~50% (loss applied inline) and each survivor is delayed (~60ms). 17/17 PASS.
netem draws its delay from a distribution table; `tc netem delay Xms Yms` ships a normal (Gaussian) table by default. Bring ubridge's delay line to the same behaviour: dl_tabledist() now consults a standard-normal inverse-CDF table (the dist!=NULL path of sch_netem.c tabledist()) instead of the uniform fallback. The table is generated once (pthread_once) via Peter Acklam's rational approximation of the normal quantile, scaled so a uniform index lookup gives a sample with std ~sigma -- so delay ~ N(latency, jitter^2), clamped at >=0 like netem. Shared across all delay lines (2 KB static, no per-instance cost). Verified statistically: delay 50 jitter 30 -> mean ~47ms, std ~28ms, ~62% within +/-1 sigma (normal is 68%; the gap is the >=0 clamp truncating the left tail, same as netem). Makefile links -lm for the quantile math. The jitter test now checks mean + spread + bounded (Gaussian stats) rather than each sample in a fixed band -- a normal distribution legitimately throws low-tail samples, so the old per-sample assertion was flaky.
test_perf.py -- the anti-GNS3#114 guarantee at scale and timing accuracy: - 400-pkt burst @50ms drains in bounded time with little loss (the serial- sleep bug took ~N*latency; the delay line ~latency). - measured delay ~= configured across 10/100/500 ms. - no-delay fast path forwards a burst quickly (regression guard). test_boundary.py -- edges: - minimum latency (1ms; setup rejects <= 0). - jitter > latency (negative draws clamp to 0, no crash). - 1-byte and ~60KB packets traverse the delay line. - stop a bridge while packets are queued -> no hang, ubridge stays responsive (cancel + release-thread join + queue free). - UBRIDGE_DELAY_LIMIT=10 boundary: exactly 10 delivered for 10 or 11 sent. Also lift the UDP traffic primitives (bound_udp / send_burst / recv_count / one_way) and build_bridge into helpers.py so the three suites share them. 34 checks total (19 latency + 6 perf + 9 boundary), stable across runs.
…ncation Three correctness fixes found by the code review (high-effort, recall-biased): 1. IOL bridge: reject the `delay` filter type explicitly. IOL listener threads call filter->handler() directly, never going through bridge_nios, and the handler is now a no-op — so a delay filter on an IOL bridge would silently do nothing. Hard-reject in cmd_add_packet_filter (hypervisor_iol_bridge.c). 2. Shared filter-list use-after-free: the bridge threads walk bridge->packet_filters unlocked, while the hypervisor thread mutates it (add/delete/reset). The existing per-packet handler walk was already racy; the new per-packet get_delay scan doubled the exposure. The hypervisor already serialises all commands under global_lock (hypervisor.c:356); this adds the matching lock in bridge_nios (filter handler loop + get_delay snapshot inside, delay-line create/destroy and pcap outside). No deadlock — hypervisor commands never call into bridge_nios. 3. dl_add_ms truncation: the ms parameter was `int` and the call site cast `(int)delay` from a potentially large `long`. Widened to `long ms`, dropped the cast, and added nsec-borrow normalisation so a negative ms (clamped path) cannot corrupt the timespec.
The delay-handler-neuter + delay-line move broke delay on IOL bridges: their listener threads call filter->handler() in their own loops, never reaching bridge_nios, so the no-op handler meant zero delay (worse than the old blocking nanosleep). Then a quick reject broke the GNS3 lab (IOU nodes legitimately apply a delay filter). Integrate the delay line into both IOL packet loops, per-port per-direction, aligned with the UDP bridge's delay line: - iol_nio_t gains delay_line_nio (NIO -> IOL) and delay_line_iol (IOL -> NIO). - Two send callbacks: iol_sendto_iol_cb (sendto the IOL instance with the port's pre-calculated IOL header) and iol_nio_send_cb (nio_send + account). - iol_delay_route() helper: lazily syncs a per-direction delay line to the snapshotted delay config (using the new delay_line_config() accessor), enqueues if delaying, returns TRUE so the caller skips its inline send. Same pattern as bridge_nios. - Both loops lock global_lock (already held by all hypervisor commands via hypervisor.c:356) around the handler walk + get_delay snapshot; delay-line create/destroy and send/enqueue are outside the lock. - Cleanup order: destroy the two delay lines (join release threads) BEFORE free_nio / close(iol_bridge_sock), in all four teardown spots (bridge-level delete, per-nio delete, reset_packet_filters, free_iol_bridges). - free_iol_bridges and cmd_delete_bridge: move close(iol_bridge_sock) to after the port loop so delay_line_nio's release thread (sendto that socket) is joined first. - bridge_nios refactored to use delay_line_config() (removes the cur_latency / cur_jitter duplicate state that the code review flagged). New delay_line_config(dl, &lat, &jit) accessor: copied from the delay line's embedded config to let a caller detect a change without keeping a parallel copy. Used by both IOL's iol_delay_route and bridge_nios. tests/delay/test_iol.py drives an IOL bridge end-to-end (fake IOL instance over netio unix-domain sockets): adds a delay filter, measures one-way latency in both directions (~100ms), and verifies teardown with an active delay doesn't hang. 37 checks total (19+6+9+3), stable.
…line details This is the first document that explains how the user-space packet filters actually behave. Covers all six filter types, the essential bidirectional (shared-filter-chain) model that produces a squaring effect for ping RTT, the delay line's netem alignment (Gaussian jitter, depth-bound tail-drop, UBRIDGE_DELAY_LIMIT), IOL support, runtime filter management, and the relationship between the `delay` filter and kernel `tc` netem.
cmd_delete_nio_udp and similar handlers destroyed delay_line_iol without NULL-ing the shared port_table pointer, while the IOL bridge listener thread (iol_bridge_listener) continued to access it outside global_lock. This caused use-after-free / double-free heap corruption, detected as malloc_printerr during thread exit when closing GNS3 nodes. Changes: - iol_bridge_listener: validate destination_nio under global_lock, skip if NULL - cmd_delete_nio_udp, create_iol_port_entry, cmd_reset_packet_filters: save pointer → NULL shared field → then delay_line_destroy (NULL-safe)
Member
|
The compilation failed on macOS, this brings the question, should we still suport this platform given where we are heading with leveraging Linux bridges for our next network back-end? |
macOS has no pthread_condattr_setclock (neither declaration nor symbol), so the unconditional call failed to compile under clang. Guard it behind #ifndef __APPLE__: on Linux/FreeBSD the cond clock is set to CLOCK_MONOTONIC (unchanged); on macOS the cond stays on its default CLOCK_REALTIME and clock_gettime is matched to it, which is that platform's only option. No behaviour change on Linux.
Contributor
Author
|
Fixed the macOS build failure (guarded pthread_condattr_setclock). Could you please approve the workflow run? But I don't have a Mac, so I cannot run the real test.
|
Contributor
Author
macOS does not have Linux kernel features like Netlink, tc/netem, or eBPF. When we move to Linux bridge, tc, and netns, macOS compatibility cannot be kept. |
Contributor
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
delaypacket filter (and the jitter, loss, and corrupt filters that compose with it) previously slept inline (nanosleep) inside the per-direction bridge thread. Each direction serviced at most ~1000/latency_mspps (~10 pps at 100 ms); any heavier offered load backed up in the kernel UDP socket buffer, RTT climbed without bound, and the link collapsed with packet loss /destination host unreachable.This was the root cause behind GNS3/gns3-server#2827 ("Packet Filters Use Incorrect Values + Cause Links to go Haywire"). The GUI and server are innocent — they only forward the configured delay value to uBridge verbatim.
The fix
Replace the inline
nanosleep()with a user-space delay line modelled on the Linux kernel netem qdisc (net/sched/sch_netem.c):time_to_send = now + delay +/- jitter): the receive thread enqueues a packet copy and never blocks, so the kernel UDP buffer stays drained.stime_to_send(cond_timedwait/CLOCK_MONOTONIC`) and sends via callback when due.tfifo-style time-ordered queue: O(1) tail append in the common in-order case, sorted insert for jitter reordering.s rational quantile approximation, matchingtc netem distribution normal. Uniform fallback matches netemsdist==NULLpath.NETEM_LIMIT_DEFAULT, overridable viaUBRIDGE_DELAY_LIMIT=...): under overload, excess packets are shed instead of buffered without bound, so latency AND memory stay bounded.add_ubridge_udp_connection->bridge start, THEN_ubridge_apply_filters). The delay config is re-read on each packet and the line is lazily (re)created/destroyed to match appearing, changing, or disappearing delay filters.filter->handlerdirectly), previously bypassed by the delay-line fix. Now both IOL directions (NIO->IOLandIOL->NIO) each get their own delay line aligned with the UDP bridge.Verified behaviour (all four GNS3 backends)
A comprehensive 100-packet test across dynamips / IOL / qemu / docker confirms:
packet_loss 50frequency_drop 4corrupt 80Flood testing (
ping -f) does NOT degrade filter behaviour — the old link-collapse (#2827) is gone. Full report:https://github.com/yueguobin/gns3-api-mcp-test/blob/main/mcp_test_docs/en/packet_filter_test_report.md
Test suite
tests/delay/— 4 suites, 37 checks, rootless (UDP only + unix IOL):test_latency.py(19 checks) — correctness: delay + jitter + loss composition + combo + runtime add/change/remove + queue limittest_perf.py(6 checks) — anti-collapse at scale (400-pkt burst @50ms drains in ~55ms; the bug took ~20s), accuracy across 10/100/500ms, no-delay baselinetest_boundary.py(9 checks) — 1ms minimum, jitter>latency clamp, 1B/60KB packet sizes, stop-with-queued doesn`t hang, limit boundary (UBRIDGE_DELAY_LIMIT=10)test_iol.py(3 checks) — IOL->NIO / NIO->IOL delay ~100ms each, teardown with delay active doesn`t hangAll suites stable across repeated runs, clean under Valgrind (no leaks, no invalid reads).
Related