Skip to content

Cleanup atomics and fix deadlock in DP bucket_can_pool() #1151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions benchmark/benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,7 @@ UMF_BENCHMARK_TEMPLATE_DEFINE(multiple_malloc_free_benchmark,
pool_allocator<disjoint_pool<os_provider>>);
UMF_BENCHMARK_REGISTER_F(multiple_malloc_free_benchmark, disjoint_pool_uniform)
->Apply(&default_multiple_alloc_uniform_size)
->Apply(&singlethreaded);
// TODO: change to multithreaded
//->Apply(&multithreaded);
->Apply(&multithreaded);

#ifdef UMF_POOL_JEMALLOC_ENABLED
UMF_BENCHMARK_TEMPLATE_DEFINE(multiple_malloc_free_benchmark, jemalloc_pool_fix,
Expand Down
86 changes: 36 additions & 50 deletions src/critnib/critnib.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (C) 2023-2024 Intel Corporation
* Copyright (C) 2023-2025 Intel Corporation
*
* Under the Apache License v2.0 with LLVM Exceptions. See LICENSE.TXT.
* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
Expand Down Expand Up @@ -133,24 +133,6 @@ struct critnib {
struct utils_mutex_t mutex; /* writes/removes */
};

/*
* atomic load
*/
static void load(void *src, void *dst) {
utils_atomic_load_acquire((word *)src, (word *)dst);
}

static void load64(uint64_t *src, uint64_t *dst) {
utils_atomic_load_acquire(src, dst);
}

/*
* atomic store
*/
static void store(void *dst, void *src) {
utils_atomic_store_release((word *)dst, (word)src);
}

/*
* internal: is_leaf -- check tagged pointer for leafness
*/
Expand Down Expand Up @@ -192,8 +174,8 @@ struct critnib *critnib_new(void) {
goto err_free_critnib;
}

VALGRIND_HG_DRD_DISABLE_CHECKING(&c->root, sizeof(c->root));
VALGRIND_HG_DRD_DISABLE_CHECKING(&c->remove_count, sizeof(c->remove_count));
utils_annotate_memory_no_check(&c->root, sizeof(c->root));
utils_annotate_memory_no_check(&c->remove_count, sizeof(c->remove_count));

return c;
err_free_critnib:
Expand Down Expand Up @@ -278,7 +260,7 @@ static struct critnib_node *alloc_node(struct critnib *__restrict c) {
struct critnib_node *n = c->deleted_node;

c->deleted_node = n->child[0];
VALGRIND_ANNOTATE_NEW_MEMORY(n, sizeof(*n));
utils_annotate_memory_new(n, sizeof(*n));

return n;
}
Expand All @@ -303,13 +285,13 @@ static void free_leaf(struct critnib *__restrict c,
*/
static struct critnib_leaf *alloc_leaf(struct critnib *__restrict c) {
if (!c->deleted_leaf) {
return umf_ba_global_alloc(sizeof(struct critnib_leaf));
return umf_ba_global_aligned_alloc(sizeof(struct critnib_leaf), 8);
}

struct critnib_leaf *k = c->deleted_leaf;

c->deleted_leaf = k->value;
VALGRIND_ANNOTATE_NEW_MEMORY(k, sizeof(*k));
utils_annotate_memory_new(k, sizeof(*k));

return k;
}
Expand All @@ -334,7 +316,7 @@ int critnib_insert(struct critnib *c, word key, void *value, int update) {
return ENOMEM;
}

VALGRIND_HG_DRD_DISABLE_CHECKING(k, sizeof(struct critnib_leaf));
utils_annotate_memory_no_check(k, sizeof(struct critnib_leaf));

k->key = key;
k->value = value;
Expand All @@ -343,10 +325,8 @@ int critnib_insert(struct critnib *c, word key, void *value, int update) {

struct critnib_node *n = c->root;
if (!n) {
store(&c->root, kn);

utils_atomic_store_release_ptr((void **)&c->root, kn);
utils_mutex_unlock(&c->mutex);

return 0;
}

Expand All @@ -361,7 +341,8 @@ int critnib_insert(struct critnib *c, word key, void *value, int update) {

if (!n) {
n = prev;
store(&n->child[slice_index(key, n->shift)], kn);
utils_atomic_store_release_ptr(
(void **)&n->child[slice_index(key, n->shift)], kn);

utils_mutex_unlock(&c->mutex);

Expand Down Expand Up @@ -396,7 +377,7 @@ int critnib_insert(struct critnib *c, word key, void *value, int update) {

return ENOMEM;
}
VALGRIND_HG_DRD_DISABLE_CHECKING(m, sizeof(struct critnib_node));
utils_annotate_memory_no_check(m, sizeof(struct critnib_node));

for (int i = 0; i < SLNODES; i++) {
m->child[i] = NULL;
Expand All @@ -406,7 +387,7 @@ int critnib_insert(struct critnib *c, word key, void *value, int update) {
m->child[slice_index(path, sh)] = n;
m->shift = sh;
m->path = key & path_mask(sh);
store(parent, m);
utils_atomic_store_release_ptr((void **)parent, m);

utils_mutex_unlock(&c->mutex);

Expand All @@ -427,7 +408,8 @@ void *critnib_remove(struct critnib *c, word key) {
goto not_found;
}

word del = (utils_atomic_increment(&c->remove_count) - 1) % DELETED_LIFE;
word del =
(utils_atomic_increment_u64(&c->remove_count) - 1) % DELETED_LIFE;
free_node(c, c->pending_del_nodes[del]);
free_leaf(c, c->pending_del_leaves[del]);
c->pending_del_nodes[del] = NULL;
Expand All @@ -436,7 +418,7 @@ void *critnib_remove(struct critnib *c, word key) {
if (is_leaf(n)) {
k = to_leaf(n);
if (k->key == key) {
store(&c->root, NULL);
utils_atomic_store_release_ptr((void **)&c->root, NULL);
goto del_leaf;
}

Expand Down Expand Up @@ -466,7 +448,8 @@ void *critnib_remove(struct critnib *c, word key) {
goto not_found;
}

store(&n->child[slice_index(key, n->shift)], NULL);
utils_atomic_store_release_ptr(
(void **)&n->child[slice_index(key, n->shift)], NULL);

/* Remove the node if there's only one remaining child. */
int ochild = -1;
Expand All @@ -482,7 +465,7 @@ void *critnib_remove(struct critnib *c, word key) {

ASSERTne(ochild, -1);

store(n_parent, n->child[ochild]);
utils_atomic_store_release_ptr((void **)n_parent, n->child[ochild]);
c->pending_del_nodes[del] = n;

del_leaf:
Expand Down Expand Up @@ -511,22 +494,23 @@ void *critnib_get(struct critnib *c, word key) {
do {
struct critnib_node *n;

load64(&c->remove_count, &wrs1);
load(&c->root, &n);
utils_atomic_load_acquire_u64(&c->remove_count, &wrs1);
utils_atomic_load_acquire_ptr((void **)&c->root, (void **)&n);

/*
* critbit algorithm: dive into the tree, looking at nothing but
* each node's critical bit^H^H^Hnibble. This means we risk
* going wrong way if our path is missing, but that's ok...
*/
while (n && !is_leaf(n)) {
load(&n->child[slice_index(key, n->shift)], &n);
utils_atomic_load_acquire_ptr(
(void **)&n->child[slice_index(key, n->shift)], (void **)&n);
}

/* ... as we check it at the end. */
struct critnib_leaf *k = to_leaf(n);
res = (n && k->key == key) ? k->value : NULL;
load64(&c->remove_count, &wrs2);
utils_atomic_load_acquire_u64(&c->remove_count, &wrs2);
} while (wrs1 + DELETED_LIFE <= wrs2);

return res;
Expand Down Expand Up @@ -597,7 +581,7 @@ static struct critnib_leaf *find_le(struct critnib_node *__restrict n,
/* recursive call: follow the path */
{
struct critnib_node *m;
load(&n->child[nib], &m);
utils_atomic_load_acquire_ptr((void **)&n->child[nib], (void **)&m);
struct critnib_leaf *k = find_le(m, key);
if (k) {
return k;
Expand All @@ -611,7 +595,7 @@ static struct critnib_leaf *find_le(struct critnib_node *__restrict n,
*/
for (; nib > 0; nib--) {
struct critnib_node *m;
load(&n->child[nib - 1], &m);
utils_atomic_load_acquire_ptr((void **)&n->child[nib - 1], (void **)&m);
if (m) {
n = m;
if (is_leaf(n)) {
Expand All @@ -635,12 +619,12 @@ void *critnib_find_le(struct critnib *c, word key) {
void *res;

do {
load64(&c->remove_count, &wrs1);
utils_atomic_load_acquire_u64(&c->remove_count, &wrs1);
struct critnib_node *n; /* avoid a subtle TOCTOU */
load(&c->root, &n);
utils_atomic_load_acquire_ptr((void **)&c->root, (void **)&n);
struct critnib_leaf *k = n ? find_le(n, key) : NULL;
res = k ? k->value : NULL;
load64(&c->remove_count, &wrs2);
utils_atomic_load_acquire_u64(&c->remove_count, &wrs2);
} while (wrs1 + DELETED_LIFE <= wrs2);

return res;
Expand Down Expand Up @@ -694,7 +678,7 @@ static struct critnib_leaf *find_ge(struct critnib_node *__restrict n,
unsigned nib = slice_index(key, n->shift);
{
struct critnib_node *m;
load(&n->child[nib], &m);
utils_atomic_load_acquire_ptr((void **)&n->child[nib], (void **)&m);
struct critnib_leaf *k = find_ge(m, key);
if (k) {
return k;
Expand All @@ -703,7 +687,7 @@ static struct critnib_leaf *find_ge(struct critnib_node *__restrict n,

for (; nib < NIB; nib++) {
struct critnib_node *m;
load(&n->child[nib + 1], &m);
utils_atomic_load_acquire_ptr((void **)&n->child[nib + 1], (void **)&m);
if (m) {
n = m;
if (is_leaf(n)) {
Expand Down Expand Up @@ -741,17 +725,19 @@ int critnib_find(struct critnib *c, uintptr_t key, enum find_dir_t dir,
}

do {
load64(&c->remove_count, &wrs1);
utils_atomic_load_acquire_u64(&c->remove_count, &wrs1);
struct critnib_node *n;
load(&c->root, &n);
utils_atomic_load_acquire_ptr((void **)&c->root, (void **)&n);

if (dir < 0) {
k = find_le(n, key);
} else if (dir > 0) {
k = find_ge(n, key);
} else {
while (n && !is_leaf(n)) {
load(&n->child[slice_index(key, n->shift)], &n);
utils_atomic_load_acquire_ptr(
(void **)&n->child[slice_index(key, n->shift)],
(void **)&n);
}

struct critnib_leaf *kk = to_leaf(n);
Expand All @@ -761,7 +747,7 @@ int critnib_find(struct critnib *c, uintptr_t key, enum find_dir_t dir,
_rkey = k->key;
_rvalue = k->value;
}
load64(&c->remove_count, &wrs2);
utils_atomic_load_acquire_u64(&c->remove_count, &wrs2);
} while (wrs1 + DELETED_LIFE <= wrs2);

if (k) {
Expand Down
2 changes: 1 addition & 1 deletion src/ipc_cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ umf_result_t umfIpcOpenedCacheGet(ipc_opened_cache_handle_t cache,

exit:
if (ret == UMF_RESULT_SUCCESS) {
utils_atomic_increment(&entry->ref_count);
utils_atomic_increment_u64(&entry->ref_count);
*retEntry = &entry->value;
}

Expand Down
6 changes: 3 additions & 3 deletions src/libumf.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@

umf_memory_tracker_handle_t TRACKER = NULL;

static unsigned long long umfRefCount = 0;
static uint64_t umfRefCount = 0;

int umfInit(void) {
if (utils_fetch_and_add64(&umfRefCount, 1) == 0) {
if (utils_fetch_and_add_u64(&umfRefCount, 1) == 0) {
utils_log_init();
TRACKER = umfMemoryTrackerCreate();
if (!TRACKER) {
Expand All @@ -54,7 +54,7 @@ int umfInit(void) {
}

void umfTearDown(void) {
if (utils_fetch_and_add64(&umfRefCount, -1) == 1) {
if (utils_fetch_and_sub_u64(&umfRefCount, 1) == 1) {
#if !defined(_WIN32) && !defined(UMF_NO_HWLOC)
umfMemspaceHostAllDestroy();
umfMemspaceHighestCapacityDestroy();
Expand Down
Loading
Loading