Cleanup atomics and fix deadlock in DP bucket_can_pool() by bratpiorka · Pull Request #1127 · oneapi-src/unified-memory-framework

bratpiorka · 2025-02-21T18:38:15Z

Cleanup atomics and replace while(true) loop in Disjoint Pool bucket_can_pool() with a pair of atomic add/sub.

fix for #1125 and #1115

This PR is required by #1143

vinser52 · 2025-02-24T09:47:15Z

@bratpiorka we have the following issue in the backlog: #886.
Might it be related to the issue you are trying to fix?

bratpiorka · 2025-02-28T10:07:05Z

@ldorau @igchor @vinser52 please re-review

bratpiorka · 2025-02-28T10:09:37Z

clang ThreadSanitizer still detects data race between:

Atomic write of size 8 at 0x7fb2f98bd6d8 by thread T15 (mutexes: write M0):
critnib_remove() at /home/runner/work/unified-memory-framework/unified-memory-framework/src/critnib/critnib.c:450
utils_atomic_store_release_ptr((void **)&n->child[slice_index(key, n->shift)], NULL);

and

Previous read of size 8 at 0x7fb2f98bd6d8 by thread T16:
find_predecessor at /home/runner/work/unified-memory-framework/unified-memory-framework/src/critnib/critnib.c:526
if (n->child[nib]) {

https://github.com/ldorau/unified-memory-framework/actions/runs/13562899026/job/37909488408

this should be fixed in the newest version of this PR

vinser52 · 2025-02-28T11:34:44Z

src/utils/utils_concurrency.h

 #include <atomic>
 #define _Atomic(X) std::atomic<X>
+
+using std::memory_order_acq_rel;


Why do we need C++ code here?

this code was indirectly included in c++ tests..
moved to test/base.hpp

@bratpiorka but C++ code is still there and in the test/base.hpp only Copyright is changed

Yes, I tried moving it to base.hpp, but that broke the building of examples. I also attempted to remove the __cplusplus code entirely, but it's not that simple. So, in fact, I'm not adding new C++ code here; I'm extending the existing code. Further refactoring should be done in a separate PR.

Yes, I tried moving it to base.hpp, but that broke the building of examples.
Why? Our examples should not use any code either from umf internals, nor tests

I also attempted to remove the __cplusplus code entirely, but it's not that simple. So, in fact, I'm not adding new C++ code here; I'm extending the existing code. Further refactoring should be done in a separate PR.
You are adding extra using, and stil we do not know why they are needed.

ldorau · 2025-02-28T11:36:40Z

This PR is required by #1143

vinser52 · 2025-02-28T11:38:16Z

This PR is required by #998 as well.

benchmark/benchmark.cpp

src/pool/pool_disjoint.c

src/utils/utils_concurrency.h

src/pool/pool_disjoint.c

src/critnib/critnib.c

lplewa · 2025-02-28T11:47:54Z

src/critnib/critnib.c

        for (nib = 0; nib <= NIB; nib++) {
-            if (n->child[nib]) {
+            struct critnib_node *m;
+            utils_atomic_load_acquire_ptr((void **)&n->child[nib], (void **)&m);
+            if (m) {
                break;
            }
        }


What is a rationale for this change. Was old code wrong / not safe?

This is needed by #1143
see: https://github.com/oneapi-src/unified-memory-framework/actions/runs/13587677512/job/37987102380

@ldorau sanitizer might report false positives. Are we sure it is a real issue?

It was needed only by #1143. After adding a lock to #1143 it is not required any more.

Which lock? What about scalability?

This lock: #1143 (comment)

It is still needed now: https://github.com/oneapi-src/unified-memory-framework/actions/runs/13650462662/job/38158581307

Can anyone explain rationale why this change is needed? I need explanation why from logical point of view, of this code. That it suppress/fixes an sanitizer bug do not explain anything. From my perspective this change is like doing random changes until sanitizer is fine.

removed
@ldorau please add this change to #1143 as it is needed (or not?) there

removed @ldorau please add this change to #1143 as it is needed (or not?) there

OK

lplewa · 2025-02-28T11:51:02Z

src/utils/utils_concurrency.h

+    ASSERT_IS_ALIGNED((uintptr_t)desired, 8);
+    ASSERT_IS_ALIGNED((uintptr_t)expected, 8);


Are you sure that this ptrs has to by 8b aligned?

https://learn.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange64

The variables for this function must be aligned on a 64-bit boundary; otherwise, this function will behave unpredictably on multiprocessor x86 systems and any non-x86 systems

We are dereferencing this pointers, so it do not matter what alignment, it has here, as anyway it their value will be copied to the register/stack before call.

Are you 100% sure that the compiler will copy these arguments and not use them directly? By the way, I really don't understand why we're discussing asserts here. In my opinion, there can never be too many asserts in the code.

We are dereferencing this pointers, so it do not matter what alignment, it has here, as anyway it their value will be copied to the register/stack before call.

As Rafal mentioned it is a requirement of the corresponding Windows API.

Furthermore, we should consider underlying HW implementation. AFAIK x86 lock instruction does not require the memory location to be aligned but there is a performance penalty if the memory location is split across multiple cache lines: in that case, instead of locking the cache line for exclusive access ht memory bus should be locked. So it is better to have aligned memory.

Guys, this function, takes second and third argument as a Value. So this do not matter PTR is aligned or not it do not matter at all, as function will get a copy values instead of the address.

LONG64 InterlockedCompareExchange64( [in, out] LONG64 volatile *Destination, [in] LONG64 ExChange, [in] LONG64 Comperand );```

ahh, sorry, I did not saw that your comment was not about destination. Of coarse, only destination should be aligned.

src/utils/utils_concurrency.h

vinser52 · 2025-03-03T16:26:22Z

src/utils/utils_concurrency.h

+    ASSERT_IS_ALIGNED((uintptr_t)ptr, 8);
+    ASSERT_IS_ALIGNED((uintptr_t)out, 8);
+    __atomic_load(ptr, out, memory_order_acquire);
+    //utils_annotate_acquire(ptr);


Why is it commented?

src/utils/utils_concurrency.h

bratpiorka · 2025-03-04T10:45:08Z

@lplewa please re-review

bratpiorka · 2025-03-04T12:12:36Z

continue in #1151

bratpiorka force-pushed the rrudnick_fix_dp_atomic branch 5 times, most recently from c570a42 to 7bd9fd3 Compare February 24, 2025 09:29

bratpiorka force-pushed the rrudnick_fix_dp_atomic branch 24 times, most recently from 6861168 to b294cc6 Compare February 25, 2025 13:36

bratpiorka force-pushed the rrudnick_fix_dp_atomic branch 7 times, most recently from 5a3a327 to 1fb932d Compare February 27, 2025 20:26

bratpiorka requested review from igchor, ldorau and vinser52 February 28, 2025 10:06

ldorau approved these changes Feb 28, 2025

View reviewed changes

vinser52 reviewed Feb 28, 2025

View reviewed changes

lplewa requested changes Feb 28, 2025

View reviewed changes

lplewa reviewed Feb 28, 2025

View reviewed changes

src/utils/utils_concurrency.h Outdated Show resolved Hide resolved

bratpiorka marked this pull request as draft February 28, 2025 14:14

bratpiorka force-pushed the rrudnick_fix_dp_atomic branch from 436aac8 to 7cba692 Compare February 28, 2025 14:16

lplewa reviewed Feb 28, 2025

View reviewed changes

src/utils/utils_concurrency.h Outdated Show resolved Hide resolved

lplewa requested changes Feb 28, 2025

View reviewed changes

src/utils/utils_concurrency.h Outdated Show resolved Hide resolved

src/utils/utils_concurrency.h Outdated Show resolved Hide resolved

src/utils/utils_concurrency.h Outdated Show resolved Hide resolved

bratpiorka force-pushed the rrudnick_fix_dp_atomic branch from 7cba692 to ca4a8f1 Compare February 28, 2025 17:10

igchor approved these changes Feb 28, 2025

View reviewed changes

vinser52 reviewed Mar 3, 2025

View reviewed changes

src/utils/utils_concurrency.h Outdated Show resolved Hide resolved

vinser52 reviewed Mar 3, 2025

View reviewed changes

src/utils/utils_concurrency.h Outdated Show resolved Hide resolved

igchor approved these changes Mar 4, 2025

View reviewed changes

		ASSERT_IS_ALIGNED((uintptr_t)desired, 8);
		ASSERT_IS_ALIGNED((uintptr_t)expected, 8);

Conversation

bratpiorka commented Feb 21, 2025 • edited by ldorau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinser52 commented Feb 24, 2025

Uh oh!

bratpiorka commented Feb 28, 2025

Uh oh!

bratpiorka commented Feb 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldorau commented Feb 28, 2025

Uh oh!

vinser52 commented Feb 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldorau Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bratpiorka Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bratpiorka commented Feb 21, 2025 •

edited by ldorau

Loading

ldorau Feb 28, 2025 •

edited

Loading

bratpiorka Mar 4, 2025 •

edited

Loading