Add identity preserving pattern to critical_sections - #2453
Conversation
github-actions
Bot
added
the
cuda.core
|
this is annoying, but at least the library and probably kernel attributes should be idempotent. But critical sections *can and will be* released (similar to the GIL although not sure what is more likely). The important thing to note here is that the final attribute setting section is self-contained and holds the lock (even if another thread may have already set the attribute or still be executing the code above).
38345fa to
1e52ae3
Compare
kkraus14
left a comment
kkraus14
left a comment
There was a problem hiding this comment.
One concurrency concern in the lazy module loader.
| if not _h_library: | ||
| HANDLE_RETURN(get_last_error()) | ||
|
|
||
| if not self._h_library: |
There was a problem hiding this comment.
critical_section is only a publication guard here, not an exactly-once initializer. create_library_handle_from_*() releases the Python thread state via GILReleaseGuard, so two callers can both reach cuLibraryLoad*() before either publishes _h_library. This second check preserves one handle identity, but the losing LibraryHandle is discarded while its deleter currently does not call cuLibraryUnload, leaving an extra CUDA library loaded for the process. Could we use a true single-flight primitive for _lazy_load_module (for example Cython's py_safe_call_once/py_safe_once_flag, or an equivalent persistent lock) and add a free-threaded concurrent-first-load regression test?
There was a problem hiding this comment.
Thanks, good point. I used the safe_call_once (I wish it was a bit prettier and didn't require except * but it seemed not too bad here).
The other places seem OK as is, although call-once could still make sense as a general pattern.
While checking, there were two places that had an incorrect order which I applied the minimal fix for (these use a pure-python pattern in Cython code where we could use atomics, but it didn't seem too important; but we could follow up with a more general audit).
| return -1 | ||
| if not self._h_library: | ||
| HANDLE_RETURN(get_last_error()) | ||
| py_safe_call_once(self._load_once, _lazy_load_module_once, <void *>self) |
There was a problem hiding this comment.
Minor, non-blocking: this introduces py_safe_call_once, but the build requirement still permits Cython 3.2.0–3.2.4. This helper had a compile failure fixed in Cython 3.2.5 (changelog). Could we raise the lower bound to Cython>=3.2.5 and align the matching pixi constraints?
2 participants
Split from gh-2194 just to shrink it a bit.
These commits are straight-forward and an attempt to guarantee identity results (not a single creation of the internal object). One of these ran into an issue in the tests. It probably doesn't matter for most, but it also doesn't hurt.
To explain what is going on: Critical sections are like the GIL. When we call arbitrary Python code they can be suspended (they definitely will be suspended in
with nogil:, i.e. when the Python thread-state is detached!).So, the callbacks can run in multiple threads, but after that we double-check and
if self.attr is None: self.attr = objis all pure C code, so only one thread can hold the critical section lock for that chunk of code.