Skip to content

Commit 80ee178

Browse files
bratpiorkabratpiorka
committed
cleanup atomics
1 parent 3fe0009 commit 80ee178

23 files changed

+259
-61
lines changed

‎cmake/helpers.cmake

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,14 @@ function(add_umf_target_compile_options name)
273273
# disable warning 6326: Potential comparison of a constant
274274
# with another constant
275275
/wd6326
276-
# disable 4200 warning: nonstandard extension used:
276+
# disable warning 28112: a variable (ptr) which is accessed
277+
# via an Interlocked function must always be accessed via an
278+
# Interlocked function
279+
/wd28112
280+
# disable warning 4324: structure was padded due to
281+
# alignment specifier
282+
/wd4324
283+
# disable warning 4200: nonstandard extension used:
277284
# zero-sized array in struct/union
278285
/wd4200)
279286
if(UMF_DEVELOPER_MODE)

‎src/ipc_cache.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ umf_result_t umfIpcOpenedCacheGet(ipc_opened_cache_handle_t cache,
232232

233233
exit:
234234
if (ret == UMF_RESULT_SUCCESS) {
235-
utils_atomic_increment(&entry->ref_count);
235+
utils_atomic_increment_u64(&entry->ref_count);
236236
*retEntry = &entry->value;
237237
}
238238

‎src/libumf.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,10 @@
2424

2525
umf_memory_tracker_handle_t TRACKER = NULL;
2626

27-
static unsigned long long umfRefCount = 0;
27+
static uint64_t umfRefCount = 0;
2828

2929
int umfInit(void) {
30-
if (utils_fetch_and_add64(&umfRefCount, 1) == 0) {
30+
if (utils_fetch_and_add_u64(&umfRefCount, 1) == 0) {
3131
utils_log_init();
3232
TRACKER = umfMemoryTrackerCreate();
3333
if (!TRACKER) {
@@ -54,7 +54,7 @@ int umfInit(void) {
5454
}
5555

5656
void umfTearDown(void) {
57-
if (utils_fetch_and_add64(&umfRefCount, -1) == 1) {
57+
if (utils_fetch_and_sub_u64(&umfRefCount, 1) == 1) {
5858
#if !defined(_WIN32) && !defined(UMF_NO_HWLOC)
5959
umfMemspaceHostAllDestroy();
6060
umfMemspaceHighestCapacityDestroy();

‎src/provider/provider_os_memory.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -587,7 +587,7 @@ static umf_result_t os_initialize(void *params, void **provider) {
587587
}
588588

589589
os_memory_provider_t *os_provider =
590-
umf_ba_global_alloc(sizeof(os_memory_provider_t));
590+
umf_ba_global_aligned_alloc(sizeof(os_memory_provider_t), 8);
591591
if (!os_provider) {
592592
return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
593593
}
@@ -934,7 +934,7 @@ static membind_t membindFirst(os_memory_provider_t *provider, void *addr,
934934

935935
if (provider->mode == UMF_NUMA_MODE_INTERLEAVE) {
936936
assert(provider->part_size != 0);
937-
size_t s = utils_fetch_and_add64(&provider->alloc_sum, size);
937+
size_t s = utils_fetch_and_add_u64(&provider->alloc_sum, size);
938938
membind.node = (s / provider->part_size) % provider->nodeset_len;
939939
membind.bitmap = provider->nodeset[membind.node];
940940
membind.bind_size = ALIGN_UP(provider->part_size, membind.page_size);

‎src/provider/provider_os_memory_internal.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2023-2024 Intel Corporation
2+
* Copyright (C) 2023-2025 Intel Corporation
33
*
44
* Under the Apache License v2.0 with LLVM Exceptions. See LICENSE.TXT.
55
* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
@@ -58,7 +58,7 @@ typedef struct os_memory_provider_t {
5858
int numa_flags; // combination of hwloc flags
5959

6060
size_t part_size;
61-
size_t alloc_sum; // sum of all allocations - used for manual interleaving
61+
uint64_t alloc_sum; // sum of all allocations - used for manual interleaving
6262

6363
struct {
6464
unsigned weight;

‎src/provider/provider_tracking.c

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -565,7 +565,7 @@ static umf_result_t trackingGetIpcHandle(void *provider, const void *ptr,
565565
return ret;
566566
}
567567

568-
cache_value->handle_id = utils_atomic_increment(&IPC_HANDLE_ID);
568+
cache_value->handle_id = utils_atomic_increment_u64(&IPC_HANDLE_ID);
569569
cache_value->ipcDataSize = ipcDataSize;
570570

571571
int insRes = critnib_insert(p->ipcCache, (uintptr_t)ptr,
@@ -703,18 +703,20 @@ static umf_result_t trackingOpenIpcHandle(void *provider, void *providerIpcData,
703703
assert(cache_entry != NULL);
704704

705705
void *mapped_ptr = NULL;
706-
utils_atomic_load_acquire(&(cache_entry->mapped_base_ptr), &mapped_ptr);
706+
utils_atomic_load_acquire_ptr(&(cache_entry->mapped_base_ptr),
707+
(void **)&mapped_ptr);
707708
if (mapped_ptr == NULL) {
708709
utils_mutex_lock(&(cache_entry->mmap_lock));
709-
utils_atomic_load_acquire(&(cache_entry->mapped_base_ptr), &mapped_ptr);
710+
utils_atomic_load_acquire_ptr(&(cache_entry->mapped_base_ptr),
711+
(void **)&mapped_ptr);
710712
if (mapped_ptr == NULL) {
711713
ret = upstreamOpenIPCHandle(p, providerIpcData,
712714
ipcUmfData->baseSize, &mapped_ptr);
713715
if (ret == UMF_RESULT_SUCCESS) {
714716
// Put to the cache
715717
cache_entry->mapped_size = ipcUmfData->baseSize;
716-
utils_atomic_store_release(&(cache_entry->mapped_base_ptr),
717-
mapped_ptr);
718+
utils_atomic_store_release_ptr(&(cache_entry->mapped_base_ptr),
719+
mapped_ptr);
718720
}
719721
}
720722
utils_mutex_unlock(&(cache_entry->mmap_lock));

‎src/utils/utils_concurrency.h

Lines changed: 112 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
#ifndef UMF_UTILS_CONCURRENCY_H
1111
#define UMF_UTILS_CONCURRENCY_H 1
1212

13+
#include <stdbool.h>
14+
#include <stdint.h>
1315
#include <stdio.h>
1416
#include <stdlib.h>
1517

@@ -19,18 +21,25 @@
1921
#include "utils_windows_intrin.h"
2022

2123
#pragma intrinsic(_BitScanForward64)
22-
#else
24+
#else /* !_WIN32 */
2325
#include <pthread.h>
2426

2527
#ifndef __cplusplus
2628
#include <stdatomic.h>
2729
#else /* __cplusplus */
2830
#include <atomic>
2931
#define _Atomic(X) std::atomic<X>
32+
33+
using std::memory_order_acq_rel;
34+
using std::memory_order_acquire;
35+
using std::memory_order_relaxed;
36+
using std::memory_order_release;
37+
3038
#endif /* __cplusplus */
3139

32-
#endif /* _WIN32 */
40+
#endif /* !_WIN32 */
3341

42+
#include "utils_common.h"
3443
#include "utils_sanitizers.h"
3544

3645
#ifdef __cplusplus
@@ -92,57 +101,123 @@ static __inline unsigned char utils_mssb_index(long long value) {
92101
}
93102

94103
// There is no good way to do atomic_load on windows...
95-
#define utils_atomic_load_acquire(object, dest) \
96-
do { \
97-
*(LONG64 *)dest = \
98-
InterlockedOr64Acquire((LONG64 volatile *)object, 0); \
99-
} while (0)
104+
static __inline void utils_atomic_load_acquire_u64(uint64_t *ptr,
105+
uint64_t *out) {
106+
// NOTE: Windows cl complains about direct accessing 'ptr' which is next
107+
// accessed using Interlocked* functions (warning 28112 - disabled)
108+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
109+
110+
// On Windows, there is no equivalent to __atomic_load, so we use cmpxchg
111+
// with 0, 0 here. This will always return the value under the pointer
112+
// without writing anything.
113+
LONG64 ret = InterlockedCompareExchange64((LONG64 volatile *)ptr, 0, 0);
114+
*out = *(uint64_t *)&ret;
115+
}
116+
117+
static __inline void utils_atomic_load_acquire_ptr(void **ptr, void **out) {
118+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
119+
uintptr_t ret = (uintptr_t)InterlockedCompareExchangePointer(ptr, 0, 0);
120+
*(uintptr_t *)out = ret;
121+
}
100122

101-
#define utils_atomic_store_release(object, desired) \
102-
InterlockedExchange64((LONG64 volatile *)object, (LONG64)desired)
123+
static __inline void utils_atomic_store_release_ptr(void **ptr, void *val) {
124+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
125+
InterlockedExchangePointer(ptr, val);
126+
}
103127

104-
#define utils_atomic_increment(object) \
105-
InterlockedIncrement64((LONG64 volatile *)object)
128+
static __inline uint64_t utils_atomic_increment_u64(uint64_t *ptr) {
129+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
130+
// return incremented value
131+
return InterlockedIncrement64((LONG64 volatile *)ptr);
132+
}
106133

107-
#define utils_atomic_decrement(object) \
108-
InterlockedDecrement64((LONG64 volatile *)object)
134+
static __inline uint64_t utils_atomic_decrement_u64(uint64_t *ptr) {
135+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
136+
// return decremented value
137+
return InterlockedDecrement64((LONG64 volatile *)ptr);
138+
}
109139

110-
#define utils_fetch_and_add64(ptr, value) \
111-
InterlockedExchangeAdd64((LONG64 *)(ptr), value)
140+
static __inline uint64_t utils_fetch_and_add_u64(uint64_t *ptr, uint64_t val) {
141+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
142+
// return the value that had previously been in *ptr
143+
return InterlockedExchangeAdd64((LONG64 volatile *)(ptr), val);
144+
}
112145

113-
// NOTE: windows version have different order of args
114-
#define utils_compare_exchange(object, desired, expected) \
115-
InterlockedCompareExchange64((LONG64 volatile *)object, *expected, *desired)
146+
static __inline uint64_t utils_fetch_and_sub_u64(uint64_t *ptr, uint64_t val) {
147+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
148+
// return the value that had previously been in *ptr
149+
// NOTE: on Windows there is no *Sub* version of InterlockedExchange
150+
return InterlockedExchangeAdd64((LONG64 volatile *)(ptr), -(LONG64)val);
151+
}
152+
153+
static __inline bool utils_compare_exchange_u64(uint64_t *ptr,
154+
uint64_t *expected,
155+
uint64_t *desired) {
156+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
157+
LONG64 out = InterlockedCompareExchange64(
158+
(LONG64 volatile *)ptr, *(LONG64 *)desired, *(LONG64 *)expected);
159+
if (out == *(LONG64 *)expected) {
160+
return true;
161+
}
162+
163+
// else
164+
*expected = out;
165+
return false;
166+
}
116167

117168
#else // !defined(_WIN32)
118169

119170
#define utils_lssb_index(x) ((unsigned char)__builtin_ctzll(x))
120171
#define utils_mssb_index(x) ((unsigned char)(63 - __builtin_clzll(x)))
121172

122-
#define utils_atomic_load_acquire(object, dest) \
123-
do { \
124-
utils_annotate_acquire((void *)object); \
125-
__atomic_load(object, dest, memory_order_acquire); \
126-
} while (0)
173+
static inline void utils_atomic_load_acquire_u64(uint64_t *ptr, uint64_t *out) {
174+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
175+
ASSERT_IS_ALIGNED((uintptr_t)out, 8);
176+
__atomic_load(ptr, out, memory_order_acquire);
177+
}
127178

128-
#define utils_atomic_store_release(object, desired) \
129-
do { \
130-
__atomic_store_n(object, desired, memory_order_release); \
131-
utils_annotate_release((void *)object); \
132-
} while (0)
179+
static inline void utils_atomic_load_acquire_ptr(void **ptr, void **out) {
180+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
181+
ASSERT_IS_ALIGNED((uintptr_t)out, 8);
182+
__atomic_load((uintptr_t *)ptr, (uintptr_t *)out, memory_order_acquire);
183+
}
133184

134-
#define utils_atomic_increment(object) \
135-
__atomic_add_fetch(object, 1, memory_order_acq_rel)
185+
static inline void utils_atomic_store_release_ptr(void **ptr, void *val) {
186+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
187+
__atomic_store_n((uintptr_t *)ptr, (uintptr_t)val, memory_order_release);
188+
}
136189

137-
#define utils_atomic_decrement(object) \
138-
__atomic_sub_fetch(object, 1, memory_order_acq_rel)
190+
static inline uint64_t utils_atomic_increment_u64(uint64_t *val) {
191+
ASSERT_IS_ALIGNED((uintptr_t)val, 8);
192+
// return incremented value
193+
return __atomic_add_fetch(val, 1, memory_order_acq_rel);
194+
}
139195

140-
#define utils_fetch_and_add64(object, value) \
141-
__atomic_fetch_add(object, value, memory_order_acq_rel)
196+
static inline uint64_t utils_atomic_decrement_u64(uint64_t *val) {
197+
ASSERT_IS_ALIGNED((uintptr_t)val, 8);
198+
// return decremented value
199+
return __atomic_sub_fetch(val, 1, memory_order_acq_rel);
200+
}
142201

143-
#define utils_compare_exchange(object, expected, desired) \
144-
__atomic_compare_exchange(object, expected, desired, 0 /* strong */, \
145-
memory_order_acq_rel, memory_order_relaxed)
202+
static inline uint64_t utils_fetch_and_add_u64(uint64_t *ptr, uint64_t val) {
203+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
204+
// return the value that had previously been in *ptr
205+
return __atomic_fetch_add(ptr, val, memory_order_acq_rel);
206+
}
207+
208+
static inline uint64_t utils_fetch_and_sub_u64(uint64_t *ptr, uint64_t val) {
209+
// return the value that had previously been in *ptr
210+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
211+
return __atomic_fetch_sub(ptr, val, memory_order_acq_rel);
212+
}
213+
214+
static inline bool utils_compare_exchange_u64(uint64_t *ptr, uint64_t *expected,
215+
uint64_t *desired) {
216+
ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
217+
return __atomic_compare_exchange(ptr, expected, desired, 0 /* strong */,
218+
memory_order_acq_rel,
219+
memory_order_relaxed);
220+
}
146221

147222
#endif // !defined(_WIN32)
148223

‎src/utils/utils_level_zero.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,11 @@
55
* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
66
*/
77

8-
#include "utils_level_zero.h"
9-
108
#include <memory>
119
#include <stdlib.h>
1210

1311
#include "utils_concurrency.h"
12+
#include "utils_level_zero.h"
1413
#include "utils_load_library.h"
1514

1615
#include "ze_api.h"

‎test/providers/cuda_helpers.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <memory>
1010
#include <stdlib.h>
1111

12+
#include "base.hpp"
1213
#include "cuda_helpers.h"
1314
#include "utils_concurrency.h"
1415
#include "utils_load_library.h"

‎test/supp/drd-umf_test-disjoint_pool.supp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
False-positive ConflictingAccess in critnib_insert
33
drd:ConflictingAccess
4-
fun:store
4+
fun:utils_atomic_store_release_ptr
55
fun:critnib_insert
66
...
77
}

‎test/supp/drd-umf_test-ipc.supp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,30 @@
55
fun:pthread_cond_destroy@*
66
...
77
}
8+
9+
{
10+
[false-positive] Double check locking pattern in trackingOpenIpcHandle
11+
drd:ConflictingAccess
12+
fun:utils_atomic_load_acquire_ptr
13+
fun:trackingOpenIpcHandle
14+
fun:umfMemoryProviderOpenIPCHandle
15+
fun:umfOpenIPCHandle
16+
...
17+
}
18+
19+
{
20+
[false-positive] trackingGetIpcHandle
21+
drd:ConflictingAccess
22+
fun:trackingGetIpcHandle
23+
fun:umfMemoryProviderGetIPCHandle
24+
fun:umfGetIPCHandle
25+
}
26+
27+
{
28+
[false-positive] trackingGetIpcHandle
29+
drd:ConflictingAccess
30+
fun:memmove
31+
fun:trackingGetIpcHandle
32+
fun:umfMemoryProviderGetIPCHandle
33+
fun:umfGetIPCHandle
34+
}

‎test/supp/drd-umf_test-jemalloc_coarse_devdax.supp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
{
1010
False-positive ConflictingAccess in critnib_insert
1111
drd:ConflictingAccess
12-
fun:store
12+
fun:utils_atomic_store_release_ptr
1313
fun:critnib_insert
1414
...
1515
}

‎test/supp/drd-umf_test-jemalloc_coarse_file.supp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
{
1010
False-positive ConflictingAccess in critnib_insert
1111
drd:ConflictingAccess
12-
fun:store
12+
fun:utils_atomic_store_release_ptr
1313
fun:critnib_insert
1414
...
1515
}

‎test/supp/drd-umf_test-provider_devdax_memory_ipc.supp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
{
22
[false-positive] Double check locking pattern in trackingOpenIpcHandle
33
drd:ConflictingAccess
4+
fun:utils_atomic_store_release_ptr
45
fun:trackingOpenIpcHandle
56
fun:umfMemoryProviderOpenIPCHandle
67
fun:umfOpenIPCHandle

0 commit comments

Comments
 (0)