Skip to content

Commit 6a2e2e8

Browse files
authored
Fix some threading issues (some free-threading related) (NVIDIA#2162)
This fixes two threading issues. * The GraphNode cleanup order is an important fix. Another thread may end up with the same pointer (but new object) as soon as we clean it up. So we have to remove it from the cache before cleaning it up. * The split mutex: This is thread-unsafe. But I am honestly not sure if that isn't just expected, or whether the mutex is good but it should also be safe from within CUDA. The PR/commit originally also included the following fixes that were split out: * Use of atomics: I think this is needed, but for this one place an atomic seemed more reasonable. (However, hard to test and if it can fail IIUC only on ARM.) * The critical sections should be pretty safe. I am not sure they will all ensure that the object is always the _identity_ but I am pretty sure it protects from worse races. (Testing did find this for MemPool.attributes, not others yet. Testing with thread-sanitizer might flush out some...) * The split mutex: This is thread-unsafe. But I am honestly not sure if that isn't just expected, or whether the mutex is good but it should also be safe from within CUDA. * Use of `setdefault` cached pattern is largely just normalizing. Without the `return dict.setdefault` a different instance may be returned on different threads (or a cache entry replaced). For the `cyGraphMemoryResource` that triggered a test with pytest-run-parallel although that doesn't mean it is problematic as such. `cuda-pathfinder` uses functools.cache, but usually for strings; the one we may want to look at is `load_nvidia_dynamic_lib`. Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
1 parent b3201ce commit 6a2e2e8

3 files changed

Lines changed: 11 additions & 6 deletions

File tree

cuda_core/cuda/core/_device_resources.pxd

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
#
33
# SPDX-License-Identifier: Apache-2.0
44

5+
cimport cython
6+
57
from cuda.bindings cimport cydriver
68
from cuda.core._resource_handles cimport ContextHandle, GreenCtxHandle
79

@@ -15,6 +17,7 @@ cdef class SMResource:
1517
unsigned int _flags
1618
bint _is_usable
1719
object __weakref__
20+
cython.pymutex _split_mutex
1821

1922
@staticmethod
2023
cdef SMResource _from_dev_resource(cydriver.CUdevResource res, int device_id)

cuda_core/cuda/core/_device_resources.pyx

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -498,10 +498,11 @@ cdef class SMResource:
498498
)
499499
_resolve_group_count(opts)
500500
_check_green_ctx_support()
501-
if _can_use_structured_sm_split():
502-
return _split_with_general_api(self, opts, dry_run)
503-
# SplitByCount requires the same 12.4+ as green ctx support (already checked above)
504-
return _split_with_count_api(self, opts, dry_run)
501+
with self._split_mutex:
502+
if _can_use_structured_sm_split():
503+
return _split_with_general_api(self, opts, dry_run)
504+
# SplitByCount requires the same 12.4+ as green ctx support (already checked above)
505+
return _split_with_count_api(self, opts, dry_run)
505506

506507

507508
cdef class WorkqueueResource:

cuda_core/cuda/core/graph/_graph_node.pyx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,11 @@ cdef class GraphNode:
161161
cdef cydriver.CUgraphNode node = as_cu(self._h_node)
162162
if node == NULL:
163163
return
164-
with nogil:
165-
HANDLE_RETURN(cydriver.cuGraphDestroyNode(node))
164+
166165
_node_registry.pop(<uintptr_t>self._h_node.get(), None)
167166
invalidate_graph_node(self._h_node)
167+
with nogil:
168+
HANDLE_RETURN(cydriver.cuGraphDestroyNode(node))
168169

169170
@property
170171
def pred(self) -> AdjacencySetProxy:

0 commit comments

Comments
 (0)