kernel: timeout: add Pugh skip list timeout backend - #117320
Conversation
a10f4ee to
21156fa
Compare
npitre
left a comment
There was a problem hiding this comment.
Reviewed the algorithm and ran it locally: builds on qemu_x86 and qemu_x86_64; kernel.timer.timeout.skiplist, timer_api (206/206) and kernel.common (80/80) pass; SMP qemu_x86_64 timeout + timer_api + sleep is 123/123 with asserts both on and off. timeout_order passes, so the same-tick FIFO claim is genuinely exercised: the skip condition there only covers the min-heap and the wheel.
I checked the invariants by hand and found no correctness problem. Levels stay order-consistent subsets of level 0, so the first node is the head's successor at every level it occupies; the equal-key walk in skiplist_predecessors_of() always reaches the node itself; sl_top cannot go stale.
The comments below share one theme: the code does not trust an invariant it actually guarantees, and pays for it in the two hottest paths. Nothing blocking.
Two things for the PR body rather than the code. With four backends already merged, the first question will be what a fifth buys, and the answer is real but unstated: this is the only backend that is simultaneously O(log n) at any horizon, unbounded (the min-heap's fixed capacity overflows fatally) and same-tick FIFO (the min-heap and wheel are not; the bucket is, but degrades to O(N) past its window). And the added coverage is the standard timeout variant, which only ever has a handful of timeouts pending, so nothing exercises the many-pending case the backend exists for. The benchmark in #104637 would give it numbers against the other four.
| /* Only splice a level if the predecessor actually points at @to. */ | ||
| static void skiplist_unlink(struct _timeout *to, struct _timeout **update) | ||
| { | ||
| uint8_t i; | ||
| uint8_t height = to->height; | ||
|
|
||
| for (i = 0; i < height; i++) { | ||
| if (update[i]->forward[i] == to) { | ||
| update[i]->forward[i] = to->forward[i]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Both callers guarantee that the predecessor points at to, so this if never fails: skiplist_predecessors_of() resolves predecessors by identity, and in z_timeout_q_pop_due() the node is the head's successor at every level it occupies. Fold the condition into an assertion and splice unconditionally. That also removes the assert-only loop in z_timeout_q_remove() (separate comment), which today re-tests exactly this.
| /* Only splice a level if the predecessor actually points at @to. */ | |
| static void skiplist_unlink(struct _timeout *to, struct _timeout **update) | |
| { | |
| uint8_t i; | |
| uint8_t height = to->height; | |
| for (i = 0; i < height; i++) { | |
| if (update[i]->forward[i] == to) { | |
| update[i]->forward[i] = to->forward[i]; | |
| } | |
| } | |
| /* Splice @to out at every level it occupies. */ | |
| static void skiplist_unlink(struct _timeout *to, struct _timeout **update) | |
| { | |
| uint8_t i; | |
| uint8_t height = to->height; | |
| for (i = 0; i < height; i++) { | |
| __ASSERT_NO_MSG(update[i]->forward[i] == to); | |
| update[i]->forward[i] = to->forward[i]; | |
| } |
There was a problem hiding this comment.
Done. Both callers already guarantee update[i]->forward[i] == to
| struct _timeout *update[SKIPLIST_LEVELS]; | ||
| bool was_first = (skiplist_first() == to); | ||
| uint8_t i; | ||
|
|
||
| __ASSERT_NO_MSG(to->height > 0); | ||
|
|
||
| skiplist_update_init(update); | ||
| skiplist_predecessors_of(to, update); | ||
|
|
||
| for (i = 0; i < to->height; i++) { | ||
| __ASSERT_NO_MSG(update[i]->forward[i] == to); | ||
| } | ||
|
|
||
| skiplist_unlink(to, update); |
There was a problem hiding this comment.
This loop's only body is an assertion. With CONFIG_ASSERT=n, __ASSERT_NO_MSG(test) expands to { } and the test expression is discarded by the preprocessor, leaving an empty bounded loop. At -Os it is optimized away, but under CONFIG_NO_OPTIMIZATIONS=y it survives and runs up to to->height empty iterations while holding the timeout spinlock (measured: z_timeout_q_remove 112 to 83 bytes). Since the check is redundant with skiplist_unlink(), moving it there keeps the assertion and drops the loop, one pass and one branch per level.
| struct _timeout *update[SKIPLIST_LEVELS]; | |
| bool was_first = (skiplist_first() == to); | |
| uint8_t i; | |
| __ASSERT_NO_MSG(to->height > 0); | |
| skiplist_update_init(update); | |
| skiplist_predecessors_of(to, update); | |
| for (i = 0; i < to->height; i++) { | |
| __ASSERT_NO_MSG(update[i]->forward[i] == to); | |
| } | |
| skiplist_unlink(to, update); | |
| struct _timeout *update[SKIPLIST_LEVELS]; | |
| bool was_first = (skiplist_first() == to); | |
| __ASSERT_NO_MSG(to->height > 0); | |
| skiplist_update_init(update); | |
| skiplist_predecessors_of(to, update); | |
| skiplist_unlink(to, update); |
| /* Earliest node, so predecessors at every occupied level are the sentinel. */ | ||
| static inline struct _timeout *z_timeout_q_pop_due(void) | ||
| { | ||
| struct _timeout *update[SKIPLIST_LEVELS]; | ||
| struct _timeout *t = skiplist_first(); | ||
|
|
||
| if ((t == NULL) || (t->abs_ticks > (int64_t)curr_tick)) { | ||
| return NULL; | ||
| } | ||
|
|
||
| skiplist_update_init(update); | ||
| skiplist_unlink(t, update); |
There was a problem hiding this comment.
The comment already states the invariant that makes update[] unnecessary here. Splicing against sl_head directly drops a SKIPLIST_LEVELS-entry stack array (64 bytes at the default 8 levels, 128 at 16), the skiplist_update_init() pass and a branch per level, on the path taken by every timeout expiry.
| /* Earliest node, so predecessors at every occupied level are the sentinel. */ | |
| static inline struct _timeout *z_timeout_q_pop_due(void) | |
| { | |
| struct _timeout *update[SKIPLIST_LEVELS]; | |
| struct _timeout *t = skiplist_first(); | |
| if ((t == NULL) || (t->abs_ticks > (int64_t)curr_tick)) { | |
| return NULL; | |
| } | |
| skiplist_update_init(update); | |
| skiplist_unlink(t, update); | |
| /* Earliest node, so it is the head's successor at every level it occupies. */ | |
| static inline struct _timeout *z_timeout_q_pop_due(void) | |
| { | |
| struct _timeout *t = skiplist_first(); | |
| uint8_t i; | |
| if ((t == NULL) || (t->abs_ticks > (int64_t)curr_tick)) { | |
| return NULL; | |
| } | |
| for (i = 0; i < t->height; i++) { | |
| __ASSERT_NO_MSG(sl_head.forward[i] == t); | |
| sl_head.forward[i] = t->forward[i]; | |
| } | |
| t->height = 0; | |
| skiplist_drop_top(); |
There was a problem hiding this comment.
Done. The earliest node is sl_head.forward[i] at every level it occupies.
| /* Numerical Recipes LCG seed; must be non-zero. */ | ||
| static uint32_t sl_rng = 2463534242U; | ||
|
|
||
| static uint8_t skiplist_random_height(void) | ||
| { | ||
| uint8_t height = 1; | ||
| uint32_t r; | ||
|
|
||
| r = sl_rng * 1664525U + 1013904223U; | ||
| sl_rng = r; | ||
|
|
||
| /* p = 1/2: each low 1-bit raises the height, capped at SKIPLIST_LEVELS. */ | ||
| while ((r & 1U) != 0U && height < SKIPLIST_LEVELS) { | ||
| height++; | ||
| r >>= 1; | ||
| } | ||
|
|
||
| return height; |
There was a problem hiding this comment.
Heights are drawn from the low bits of an LCG modulo 2^32. Bit 0 of such an LCG has period 2, so it strictly alternates: every second node gets height 1 and the height sequence is a fixed cycle (2,1,3,1,2,1,4,1,...) rather than independent draws. The marginal distribution is still exactly 50/25/12.5/6.25/..., so search cost is unaffected and this is not a bug today, but the comment claims randomness the generator does not provide, and the low-bit choice will bite if p or MAX_LEVEL changes.
xorshift32 has usable low bits and needs no multiply, which would only matter if this is ever used on a Cortex-M0. 2463534242 is already Marsaglia's canonical xorshift32 seed rather than a Numerical Recipes one, so the comment becomes accurate. u32_count_trailing_zeros() replaces the shift loop and is already used by timeout_wheel.h and timeout_bucket.h; it returns 32 for 0, which the MIN caps. Measured over 200k draws after the switch: 49.99/24.90/12.67/6.26/3.10/1.54/0.77/0.78 %, with the alternation gone.
| /* Numerical Recipes LCG seed; must be non-zero. */ | |
| static uint32_t sl_rng = 2463534242U; | |
| static uint8_t skiplist_random_height(void) | |
| { | |
| uint8_t height = 1; | |
| uint32_t r; | |
| r = sl_rng * 1664525U + 1013904223U; | |
| sl_rng = r; | |
| /* p = 1/2: each low 1-bit raises the height, capped at SKIPLIST_LEVELS. */ | |
| while ((r & 1U) != 0U && height < SKIPLIST_LEVELS) { | |
| height++; | |
| r >>= 1; | |
| } | |
| return height; | |
| /* Marsaglia xorshift32 state; period 2^32-1, must stay non-zero. */ | |
| static uint32_t sl_rng = 2463534242U; | |
| static uint8_t skiplist_random_height(void) | |
| { | |
| uint32_t r = sl_rng; | |
| r ^= r << 13; | |
| r ^= r >> 17; | |
| r ^= r << 5; | |
| sl_rng = r; | |
| /* p = 1/2: height is 1 + the run of low 1-bits, capped at SKIPLIST_LEVELS. */ | |
| return 1 + MIN(u32_count_trailing_zeros(~r), SKIPLIST_LEVELS - 1); |
There was a problem hiding this comment.
Had no idea on this,
thanks for sharing this.
Have updated the same.
| Each pending timeout carries CONFIG_TIMEOUT_SKIPLIST_MAX_LEVEL | ||
| forward pointers, so per-event RAM is higher than the delta list | ||
| or min-heap. Requires 64-bit timeouts. Consider this when many | ||
| timeouts are pending and a bounded heap is undesirable. |
There was a problem hiding this comment.
Worth quantifying, since this is the main thing an integrator weighs against the min-heap, the other O(log n) option. Measured from DWARF on qemu_x86_64: struct _timeout is 88 bytes here against 32 for the delta list and 24 for the min-heap, and struct k_thread goes from 880 to 928. The gap is large because expected height is 2 while every node statically carries all 8 levels, so most of each tower is unused.
| Each pending timeout carries CONFIG_TIMEOUT_SKIPLIST_MAX_LEVEL | |
| forward pointers, so per-event RAM is higher than the delta list | |
| or min-heap. Requires 64-bit timeouts. Consider this when many | |
| timeouts are pending and a bounded heap is undesirable. | |
| Each pending timeout carries CONFIG_TIMEOUT_SKIPLIST_MAX_LEVEL | |
| forward pointers, sized for the worst case although the expected | |
| height is 2. At the default 8 levels struct _timeout is 88 bytes | |
| on a 64-bit target, against 32 for the delta list and 24 for the | |
| min-heap, and every k_thread, k_timer and delayable work item | |
| pays it. Requires 64-bit timeouts. Consider this when many | |
| timeouts are pending and a bounded heap is undesirable. |
There was a problem hiding this comment.
Done partially. but will it make sense to keep these sizes in the Kconfig help, or should they live somewhere that can stay accurate?
The RAM cost is the real trade-off here, so a number is useful. The current wording mixes a 64-bit _timeout comparison (88 vs 32 dlist / 24 min-heap at 8 levels) with a k_thread figure that is qemu_x86_64-specific (880 -> 928). The thread delta is not even sizeof(_timeout): FXSAVE 16-byte alignment swallows 8 bytes, so Cortex-M sees the full +56.
If we keep numbers in Kconfig, I would rather quote only struct _timeout and drop the board-specific k_thread line. A one-line comparison in the docs (or next to TIMEOUT_SKIPLIST_MAX_LEVEL) would also age better than baking 880/928 into the choice help.
This commit introduces a new timeout backend using a Pugh skip list, which allows for O(log N) insertion and removal of timeouts. The skip list maintains a same-tick firing order and has no capacity limit, making it suitable for systems with many concurrent timeouts. The implementation includes necessary Kconfig options and updates to the timeout management code to support this new backend. Signed-off-by: Dhruv Menon <dhruvmenon1104@gmail.com>
21156fa to
ab8ac76
Compare
Thanks for the local run and the invariant check. The hot-path nits are in: I have updated the PR body as to why we need the fifth-backend, (O(log n) at any horizon, unbounded, same-tick FIFO) and kernel.timer.timeout.skiplist never queued a real timeout list, it only ran K_TIMEOUT_SUM arithmetic. Skiplist’s reason to exist is many pending nodes, so that variant could not catch a broken insert, unlink, or FIFO walk |
npitre
left a comment
There was a problem hiding this comment.
should I create test cases for that?
Yes, and it is worth knowing you would be first: tests/kernel/timeout only exercises K_TIMEOUT_SUM arithmetic, so the min_heap, wheel and bucket scenarios there prove those backends build and nothing more. Nothing in the tree queues many timeouts under a non-default backend. Write it generically, add the skip list as one scenario, and leave the other four to whoever wants them.
Skiplist's reason to exist is many pending nodes, so that variant could not catch a broken insert, unlink, or FIFO walk
Enough to catch those three: queue a few hundred timeouts with scrambled expiries including same-tick clusters, assert they fire in expiry order and that same-tick ones fire in insertion order, and abort a scattered subset mid-flight, so arbitrary removal and the equal-key walk in skiplist_predecessors_of() are exercised and not just head removal. Keep the count under CONFIG_TIMEOUT_HEAP_MAX_ENTRIES if you want the min-heap to run it too.
All six cases at N=1000 (average cycles):
Abort exp and Add dec in that table are not measuring your backend. They are the only two patterns where every operation changes the earliest deadline, so each one pays a sys_clock_set_timeout(). Suppress that call and, at 500 timeouts on qemu_x86_64, abort-in-expiry-order drops 19x and add-decreasing 9x (13709 to 714 and 13923 to 1469 cycles), while the other four columns move by under 10 percent. Head removal then costs less than mid-list removal, which is what the structure predicts.
The wheel's apparent 10x lead in those two columns is the same effect inverted. timeout_wheel_first() scans only the soon lists, so for anything past that window both insert and remove report false and the wheel never reprograms. It is not doing less structural work, it is skipping the clock write your other backends pay.
So the table as published invites the conclusion that the skip list is as slow as the delta list on abort, which is not what it measured. Worth a line under it, or splitting the clock write out of the figures.
This commit introduces a new timeout backend using a Pugh skip list, which allows for O(log N) insertion and removal of timeouts. The skip list maintains a same-tick firing order and has no capacity limit, making it suitable for systems with many concurrent timeouts. The implementation includes necessary Kconfig options and updates to the timeout management code to support this new backend.
Enable with
CONFIG_TIMEOUT_BACKEND_SKIPLIST=y(requiresCONFIG_TIMEOUT_64BIT). Height isCONFIG_TIMEOUT_SKIPLIST_MAX_LEVEL(default 8).Why a fifth backend
Four backends are already in tree. This is the only one that is simultaneously:
dlist is FIFO and unbounded, but insertion is O(N) when the new wait is later than everything already queued. Wheel is O(1) only inside ~1024 ticks. Bucket is O(1) only inside
CONFIG_TIMEOUT_BUCKET_LISTSticks (default 32).The cost is RAM: at 8 levels,
struct _timeoutis 88 bytes on a 64-bit target, against 32 for the delta list and 24 for the min-heap. Onqemu_x86_64,struct k_threadgoes 880 → 928. Everyk_timerand delayable work item pays the same per-node increase. Expected node height is 2; the extra pointers are worst-case.Benchmark using
timeout_mgmtqemu_x86_64/atom, SMP=2, TSC, no icount. Cycles are QEMU TCG, not silicon. N=100 is 1000 iterations; N≥250 is 200.Add with increasing waits (dlist’s worst case; avg / max):
Dlist grew ~4× as N went 10×. Skiplist stayed ~480–550 (about 5.4x cheaper than dlist at N=1000), still unbounded and FIFO. Wheel and min-heap stay flat on this pattern; bucket already goes linear once waits miss the 32-tick window.
All six cases at N=1000 (average cycles):
Testing
kernel.timer.timeout.skiplisttimer_api(206/206) andkernel.common(80/80) onqemu_x86/qemu_x86_64qemu_x86_64timeout +timer_api+ sleep: 123/123 with asserts on and offtimeout_order(same-tick FIFO; that test skips only min-heap and wheel)