From a4f3eb750dd1a7eb382132648d6ac7270f0a7d2e Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Sun, 19 Jul 2026 12:12:03 -0700 Subject: [PATCH 1/2] ck_rhs: fix Robin Hood probe-bound edge cases Allocation failure surfaced an edge case where robin-hood probing limit could become inaccurate. The symptom we were seeing was the following: ```c writeLock(); k = ck_rhs_get(hs, h, k); d = ck_rhs_remove(hs, h, k); assert(d == k); // fails because `d == NULL` ``` Standalone reproducer: https://gist.github.com/michael-grunder/516e6ceb0b93b5f02f7aaac259f9e8d9 The symptom was more likely in low memory conditions causing allocation failures but could occur without them. The following three conditions could lead to the error: 1. A full Robin Hood relocation history switched to a probe mode that restarted the search instead of continuing the current relocation. 2. A successful Robin Hood relocation could leave its original slot marked `in_rh` erroneously. 3. Backward-shift deletion reconstructed a candidate's home bucket with a probe count that was one too small. This could lower a probe bound while a live entry remained beyond it. A new `probe_bound.c` regression test is introduced that attempts to surface all of the above edge cases. Because some of the tests need to peek into private `ck_rhs_map` struct members it includes the actual `src/ck_rhs.c` file not just the header. This commit also includes a small refactor of logic after we are certain the map just grew on us. Previously in these cases the code was setting `in_rh = false` which was not required since the larger reallocated map always has these entries initialized to false. --- .gitignore | 1 + regressions/ck_rhs/validate/Makefile | 7 +- regressions/ck_rhs/validate/probe_bound.c | 310 ++++++++++++++++++++++ src/ck_rhs.c | 45 +++- 4 files changed, 348 insertions(+), 15 deletions(-) create mode 100644 regressions/ck_rhs/validate/probe_bound.c diff --git a/.gitignore b/.gitignore index f4f38354..a098f8bb 100644 --- a/.gitignore +++ b/.gitignore @@ -121,6 +121,7 @@ regressions/ck_queue/validate/ck_stailq regressions/ck_rhs/benchmark/parallel_bytestring regressions/ck_rhs/benchmark/serial regressions/ck_rhs/validate/serial +regressions/ck_rhs/validate/probe_bound regressions/ck_ring/benchmark/latency regressions/ck_ring/validate/ck_ring_mpmc regressions/ck_ring/validate/ck_ring_mpmc_template diff --git a/regressions/ck_rhs/validate/Makefile b/regressions/ck_rhs/validate/Makefile index 5987395f..7498667d 100644 --- a/regressions/ck_rhs/validate/Makefile +++ b/regressions/ck_rhs/validate/Makefile @@ -1,13 +1,18 @@ .PHONY: check clean distribution -OBJECTS=serial +OBJECTS=probe_bound serial all: $(OBJECTS) serial: serial.c ../../../include/ck_rhs.h ../../../src/ck_rhs.c $(CC) $(CFLAGS) -o serial serial.c ../../../src/ck_rhs.c +# probe_bound.c includes ../../../src/ck_rhs.c directly; see the note there. +probe_bound: probe_bound.c ../../../include/ck_rhs.h ../../../src/ck_rhs.c + $(CC) $(CFLAGS) -o probe_bound probe_bound.c + check: all + ./probe_bound ./serial clean: diff --git a/regressions/ck_rhs/validate/probe_bound.c b/regressions/ck_rhs/validate/probe_bound.c new file mode 100644 index 00000000..fa02f775 --- /dev/null +++ b/regressions/ck_rhs/validate/probe_bound.c @@ -0,0 +1,310 @@ +/* + * Copyright 2026 Michael Grunder. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include +#include +#include +#include + +/* + * Unlike serial.c, this test includes the implementation rather than linking + * against it. Asserting that no descriptor is left marked in_rh requires the + * private map layout, and that invariant has no black box equivalent: a leaked + * in_rh only degrades Robin Hood balancing, so every key stays reachable. + */ +#include "../../../src/ck_rhs.c" + +#include "../../common.h" + +#define INITIAL_CAPACITY 32768 +#define INITIAL_KEYS 23000 +#define CHURN_OPERATIONS 7000 +#define MAX_KEYS (INITIAL_KEYS + CHURN_OPERATIONS) +#define HASH_MASK UINT64_C(0x3fff) + +struct key { + unsigned long hash; + unsigned long id; +}; + +static struct key keys[MAX_KEYS]; +static unsigned int active[INITIAL_KEYS]; +static bool fail_allocations; +static unsigned long failed_allocations; +static void *deferred_free; + +static void * +test_malloc(size_t size) +{ + + if (fail_allocations) { + failed_allocations++; + return NULL; + } + + return malloc(size); +} + +static void +test_free(void *pointer, size_t size, bool defer) +{ + + (void)size; + if (defer) { + if (deferred_free != NULL) + ck_error("ERROR: More than one deferred free is pending.\n"); + deferred_free = pointer; + return; + } + + free(pointer); + return; +} + +static struct ck_malloc allocator = { + .malloc = test_malloc, + .free = test_free +}; + +static unsigned long +key_hash(const void *object, unsigned long seed) +{ + const struct key *key = object; + + (void)seed; + return key->hash; +} + +static bool +key_compare(const void *left, const void *right) +{ + const struct key *a = left; + const struct key *b = right; + + return a->id == b->id; +} + +static uint64_t +random_next(uint64_t *state) +{ + uint64_t z; + + z = (*state += UINT64_C(0x9e3779b97f4a7c15)); + z = (z ^ (z >> 30)) * UINT64_C(0xbf58476d1ce4e5b9); + z = (z ^ (z >> 27)) * UINT64_C(0x94d049bb133111eb); + return z ^ (z >> 31); +} + +/* + * No descriptor may be left marked in_rh once an operation has completed. A + * slot marked in_rh is skipped when choosing a Robin Hood relocation victim, + * so a leaked mark permanently excludes that slot from rebalancing. + */ +static void +validate_no_in_rh(ck_rhs_t *rhs, unsigned long operation) +{ + struct ck_rhs_map *map = rhs->map; + unsigned long i, marked = 0; + + for (i = 0; i <= map->mask; i++) { + if (ck_rhs_in_rh(map, i) == true) + marked++; + } + + if (marked != 0) { + ck_error("ERROR: %lu descriptor(s) left marked in_rh after " + "operation %lu.\n", marked, operation); + } + + return; +} + +enum grow_operation { + GROW_SET, + GROW_APPLY, + GROW_OPERATIONS +}; + +struct apply_state { + struct key *replacement; + void *previous; + unsigned int calls; +}; + +static void * +apply_replace(void *previous, void *closure) +{ + struct apply_state *state = closure; + + state->previous = previous; + state->calls++; + return state->replacement; +} + +static void +validate_successful_grow(enum grow_operation operation) +{ + static const char *names[] = { "set", "apply" }; + struct key test[] = { + { 0, 0 }, + { 1, 1 }, + { 0, 2 }, + { 0, 2 } + }; + struct apply_state state = { &test[3], NULL, 0 }; + unsigned long capacity; + ck_rhs_t rhs; + void *previous = NULL; + bool result = false; + + if (ck_rhs_init(&rhs, CK_RHS_MODE_SPMC | CK_RHS_MODE_OBJECT, + key_hash, key_compare, &allocator, 8, 0) == false) { + ck_error("ERROR: Failed to initialize RHS for successful %s grow.\n", + names[operation]); + } + + /* + * Put the target at probe distance three without Robin Hood balancing. + * Its normal probe selects test[1] as a relocation candidate; limiting + * that relocation to one probe forces ck_rhs_put_robin_hood to grow. + */ + if (ck_rhs_put(&rhs, test[0].hash, &test[0]) == false || + ck_rhs_put(&rhs, test[1].hash, &test[1]) == false || + ck_rhs_put_internal(&rhs, test[2].hash, &test[2], + CK_RHS_PROBE_NO_RH) == false) { + ck_error("ERROR: Failed to prepare successful %s grow.\n", + names[operation]); + } + + capacity = rhs.map->capacity; + rhs.map->probe_limit = 1; + switch (operation) { + case GROW_SET: + result = ck_rhs_set(&rhs, test[3].hash, &test[3], &previous); + break; + case GROW_APPLY: + result = ck_rhs_apply(&rhs, test[3].hash, &test[3], apply_replace, + &state); + previous = state.previous; + break; + default: + ck_error("ERROR: Invalid successful grow operation.\n"); + } + + if (result == false || rhs.map->capacity <= capacity || + previous != &test[2] || + ck_rhs_get(&rhs, test[3].hash, &test[3]) != &test[3]) { + ck_error("ERROR: Successful %s grow produced an invalid result.\n", + names[operation]); + } + if (operation == GROW_APPLY && state.calls != 1) + ck_error("ERROR: Apply callback invoked %u times across growth.\n", + state.calls); + validate_no_in_rh(&rhs, operation); + ck_rhs_destroy(&rhs); + free(deferred_free); + deferred_free = NULL; + return; +} + +static void +validate_active(ck_rhs_t *rhs, unsigned long operation) +{ + unsigned int i; + + for (i = 0; i < INITIAL_KEYS; i++) { + unsigned int key_index; + + key_index = active[i]; + if (ck_rhs_get(rhs, keys[key_index].hash, &keys[key_index]) != + &keys[key_index]) { + ck_error("ERROR: Active key %u is unreachable after operation " + "%lu.\n", key_index, operation); + } + } +} + +int +main(void) +{ + struct ck_rhs_stat stat; + uint64_t random = 1; + ck_rhs_t rhs; + struct key *found; + unsigned int next_key; + unsigned long i; + unsigned int operation; + + for (operation = 0; operation < GROW_OPERATIONS; operation++) + validate_successful_grow(operation); + + if (ck_rhs_init(&rhs, CK_RHS_MODE_SPMC | CK_RHS_MODE_OBJECT, + key_hash, NULL, &allocator, INITIAL_CAPACITY, 0) == false) { + ck_error("ERROR: Failed to initialize RHS.\n"); + } + + fail_allocations = true; + for (i = 0; i < INITIAL_KEYS; i++) { + keys[i].hash = random_next(&random) & HASH_MASK; + active[i] = i; + if (ck_rhs_put(&rhs, keys[i].hash, &keys[i]) == false) + ck_error("ERROR: Failed to insert initial key %lu.\n", i); + } + validate_active(&rhs, 0); + validate_no_in_rh(&rhs, 0); + + for (i = 0, next_key = INITIAL_KEYS; i < CHURN_OPERATIONS; i++, next_key++) { + unsigned int active_offset = random_next(&random) % INITIAL_KEYS; + unsigned int old_key = active[active_offset]; + + found = ck_rhs_get(&rhs, keys[old_key].hash, &keys[old_key]); + if (found != &keys[old_key]) { + ck_error("ERROR: Failed to find key %u before removal at " + "operation %lu.\n", old_key, i); + } + if (ck_rhs_remove(&rhs, found->hash, found) != found) { + ck_error("ERROR: Failed to remove key %u at operation %lu.\n", + old_key, i); + } + + keys[next_key].hash = random_next(&random) & HASH_MASK; + if (ck_rhs_put(&rhs, keys[next_key].hash, &keys[next_key]) == false) { + ck_error("ERROR: Failed to insert key %u at operation %lu.\n", + next_key, i); + } + active[active_offset] = next_key; + validate_no_in_rh(&rhs, i); + } + + validate_active(&rhs, CHURN_OPERATIONS); + if (failed_allocations == 0) + ck_error("ERROR: No allocation failure was exercised.\n"); + ck_rhs_stat(&rhs, &stat); + printf("probe maximum: %u\n", stat.probe_maximum); + + ck_rhs_destroy(&rhs); + return 0; +} diff --git a/src/ck_rhs.c b/src/ck_rhs.c index 9f281699..25d40f44 100644 --- a/src/ck_rhs.c +++ b/src/ck_rhs.c @@ -82,6 +82,7 @@ enum ck_rhs_probe_behavior { CK_RHS_PROBE_INSERT, /* Short-circuit on probe bound if tombstone found. */ CK_RHS_PROBE_ROBIN_HOOD,/* Look for the first slot available for the entry we are about to replace, only used to internally implement Robin Hood */ + CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE, /* Continue a Robin Hood probe without selecting another relocation candidate. */ CK_RHS_PROBE_NO_RH, /* Don't do the RH dance */ }; struct ck_rhs_entry_desc { @@ -650,7 +651,8 @@ ck_rhs_map_probe_rm(struct ck_rhs *hs, compare = key; #endif *object = NULL; - if (behavior != CK_RHS_PROBE_ROBIN_HOOD) { + if (behavior != CK_RHS_PROBE_ROBIN_HOOD && + behavior != CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE) { probes = 0; offset = h & map->mask; } else { @@ -677,7 +679,8 @@ ck_rhs_map_probe_rm(struct ck_rhs *hs, if (k == CK_RHS_EMPTY) goto leave; - if (behavior != CK_RHS_PROBE_NO_RH) { + if (behavior != CK_RHS_PROBE_NO_RH && + behavior != CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE) { struct ck_rhs_entry_desc *desc = (void *)&map->entries.no_entries.descs[offset]; if (pr == -1 && @@ -693,7 +696,8 @@ ck_rhs_map_probe_rm(struct ck_rhs *hs, } } - if (behavior != CK_RHS_PROBE_ROBIN_HOOD) { + if (behavior != CK_RHS_PROBE_ROBIN_HOOD && + behavior != CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE) { #ifdef CK_RHS_PP if (hs->mode & CK_RHS_MODE_OBJECT) { if (((uintptr_t)k >> CK_MD_VMA_BITS) != hv) { @@ -763,7 +767,8 @@ ck_rhs_map_probe(struct ck_rhs *hs, #endif *object = NULL; - if (behavior != CK_RHS_PROBE_ROBIN_HOOD) { + if (behavior != CK_RHS_PROBE_ROBIN_HOOD && + behavior != CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE) { probes = 0; offset = h & map->mask; } else { @@ -791,7 +796,8 @@ ck_rhs_map_probe(struct ck_rhs *hs, k = ck_pr_load_ptr(&map->entries.descs[offset].entry); if (k == CK_RHS_EMPTY) goto leave; - if ((behavior != CK_RHS_PROBE_NO_RH)) { + if (behavior != CK_RHS_PROBE_NO_RH && + behavior != CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE) { struct ck_rhs_entry_desc *desc = &map->entries.descs[offset]; if (pr == -1 && @@ -807,7 +813,8 @@ ck_rhs_map_probe(struct ck_rhs *hs, } } - if (behavior != CK_RHS_PROBE_ROBIN_HOOD) { + if (behavior != CK_RHS_PROBE_ROBIN_HOOD && + behavior != CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE) { #ifdef CK_RHS_PP if (hs->mode & CK_RHS_MODE_OBJECT) { if (((uintptr_t)k >> CK_MD_VMA_BITS) != hv) { @@ -978,7 +985,7 @@ ck_rhs_put_robin_hood(struct ck_rhs *hs, slot = map->probe_func(hs, map, &n_probes, &first, h, key, &object, map->probe_limit, prevs_nb == CK_RHS_MAX_RH ? - CK_RHS_PROBE_NO_RH : CK_RHS_PROBE_ROBIN_HOOD); + CK_RHS_PROBE_ROBIN_HOOD_NO_RELOCATE : CK_RHS_PROBE_ROBIN_HOOD); if (slot == -1 && first == -1) { if (ck_rhs_grow(hs, map->capacity << 1) == false) { @@ -1036,6 +1043,7 @@ ck_rhs_put_robin_hood(struct ck_rhs *hs, desc->in_rh = false; desc = ck_rhs_desc(map, orig_slot); } + ck_rhs_unset_rh(map, orig_slot); return 0; } @@ -1097,7 +1105,9 @@ ck_rhs_do_backward_shift_delete(struct ck_rhs *hs, long slot) tmp_offset = ck_rhs_map_probe_next(map, offset, probe); while (probe < max_probes) { - if (h == (unsigned long)ck_rhs_get_first_offset(map, tmp_offset, probe)) + /* probe_next advances to probe + 1. */ + if (h == (unsigned long)ck_rhs_get_first_offset(map, + tmp_offset, probe + 1)) break; probe++; tmp_offset = ck_rhs_map_probe_next(map, tmp_offset, probe); @@ -1150,10 +1160,15 @@ ck_rhs_fas(struct ck_rhs *hs, desc2 = ck_rhs_desc(map, first); desc->in_rh = true; ret = ck_rhs_put_robin_hood(hs, first, desc2); - desc->in_rh = false; + /* + * A return of 1 means the table was grown, so map and every + * descriptor taken from it now belong to the retired map. + * The replacement map starts with in_rh clear throughout. + */ if (CK_CC_UNLIKELY(ret == 1)) goto restart; - else if (CK_CC_UNLIKELY(ret != 0)) + desc->in_rh = false; + if (CK_CC_UNLIKELY(ret != 0)) return false; ck_pr_store_ptr(ck_rhs_entry_addr(map, first), insert); ck_pr_inc_uint(&map->generation[h & CK_RHS_G_MASK]); @@ -1243,11 +1258,12 @@ ck_rhs_apply(struct ck_rhs *hs, } desc2 = ck_rhs_desc(map, first); int ret = ck_rhs_put_robin_hood(hs, first, desc2); + /* See the note in ck_rhs_fas regarding the retired map. */ + if (CK_CC_UNLIKELY(ret == 1)) + goto restart; if (slot != -1) desc->in_rh = false; - if (CK_CC_UNLIKELY(ret == 1)) - goto restart; if (CK_CC_UNLIKELY(ret == -1)) return false; /* If an earlier bucket was found, then store entry there. */ @@ -1320,11 +1336,12 @@ ck_rhs_set(struct ck_rhs *hs, } desc2 = ck_rhs_desc(map, first); int ret = ck_rhs_put_robin_hood(hs, first, desc2); + /* See the note in ck_rhs_fas regarding the retired map. */ + if (CK_CC_UNLIKELY(ret == 1)) + goto restart; if (slot != -1) desc->in_rh = false; - if (CK_CC_UNLIKELY(ret == 1)) - goto restart; if (CK_CC_UNLIKELY(ret == -1)) return false; /* If an earlier bucket was found, then store entry there. */ From 9184dc48c280637776c546204e15c96eb8698152 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Mon, 20 Jul 2026 13:56:36 -0700 Subject: [PATCH 2/2] ck_rhs: preserve ck_rhs_fas replacement across growth The ck_rhs_fas function can erroneously return success without actually replacing the entry when Robin Hood relocation grows the table. After growth, subsequent probing continued using the old, newly retired map, causing the replacement to be lost. The fix is simple. Just update the map pointer when we jump to `restart`. Reproducer: https://gist.github.com/michael-grunder/b0d396001843b46aaa89b58fefd99819 --- regressions/ck_rhs/validate/probe_bound.c | 6 +++++- src/ck_rhs.c | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/regressions/ck_rhs/validate/probe_bound.c b/regressions/ck_rhs/validate/probe_bound.c index fa02f775..90604186 100644 --- a/regressions/ck_rhs/validate/probe_bound.c +++ b/regressions/ck_rhs/validate/probe_bound.c @@ -143,6 +143,7 @@ validate_no_in_rh(ck_rhs_t *rhs, unsigned long operation) } enum grow_operation { + GROW_FAS, GROW_SET, GROW_APPLY, GROW_OPERATIONS @@ -167,7 +168,7 @@ apply_replace(void *previous, void *closure) static void validate_successful_grow(enum grow_operation operation) { - static const char *names[] = { "set", "apply" }; + static const char *names[] = { "fas", "set", "apply" }; struct key test[] = { { 0, 0 }, { 1, 1 }, @@ -202,6 +203,9 @@ validate_successful_grow(enum grow_operation operation) capacity = rhs.map->capacity; rhs.map->probe_limit = 1; switch (operation) { + case GROW_FAS: + result = ck_rhs_fas(&rhs, test[3].hash, &test[3], &previous); + break; case GROW_SET: result = ck_rhs_set(&rhs, test[3].hash, &test[3], &previous); break; diff --git a/src/ck_rhs.c b/src/ck_rhs.c index 25d40f44..d9d638a7 100644 --- a/src/ck_rhs.c +++ b/src/ck_rhs.c @@ -1139,11 +1139,13 @@ ck_rhs_fas(struct ck_rhs *hs, const void *object; const void *insert; unsigned long n_probes; - struct ck_rhs_map *map = hs->map; + struct ck_rhs_map *map; struct ck_rhs_entry_desc *desc, *desc2; *previous = NULL; restart: + map = hs->map; + slot = map->probe_func(hs, map, &n_probes, &first, h, key, &object, ck_rhs_map_bound_get(map, h), CK_RHS_PROBE);