Repository navigation
[Common] Add explicit CUDA state lifecycle APIs for in-process restarts - #3779
Oleg-Goncharov wants to merge 11 commits into
Conversation
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
Signed-off-by: Oleg Goncharov <ogoncharov@nvidia.com>
There was a problem hiding this comment.
The scope of this PR is a bit larger than what we'd like. From what I've seen here, I don't trust Codex's code quality and I'd like a human to properly go through and vouch that they fully understand it. I wonder if we could undo all the API changes by making a minor tweak: #3779 (comment)
The core infrastructure for resource invalidation is pretty nice though. I have some suggestions to make it even more cute, especially #3779 (comment).
There was a problem hiding this comment.
Is there value in forcing all workspaces to use the same infrastructure? Workspace requirements are kernel-dependent and alignment management is not that challenging.
| NVTE_CHECK(bytes <= std::numeric_limits<size_t>::max() - offset, | ||
| "Workspace block size overflows size_t."); |
There was a problem hiding this comment.
Nit: These size_t overflow checks scattered through the code don't really accomplish much. We'll never approach sizes that large unless we are already in a weird buggy state.
| NVTE_CHECK(alignment != 0 && (alignment & (alignment - 1)) == 0, | ||
| "Workspace alignment must be a nonzero power of two."); |
There was a problem hiding this comment.
Nit: Why does alignment need to be a power of 2? It's not worth putting extra effort to support non-powers-of-2, but also it's not worth putting extra effort disallowing it.
| // Minimum alignment of the allocation and every nonempty block. An operation | ||
| // may request a stricter power-of-two alignment for an individual block. | ||
| constexpr size_t kWorkspaceAlignment = 128; |
There was a problem hiding this comment.
Why not allow smaller than 128 byte alignment? If this is general workspace infrastructure, I could imagine situations like storing a few floats or ints in the workspace.
| static_assert(std::is_trivially_copyable<T>::value, | ||
| "Workspace elements must be trivially copyable."); |
There was a problem hiding this comment.
Why does T need to be trivially copyable? Isn't it valid to initialize the workspace by calling a constructor within a kernel? In fact, don't we do something like that for TMA descriptors?
| * Device must be a nonnegative CUDA ordinal, supplied by the caller because | ||
| * CUDA may be unavailable during recovery. Repeated resets are safe. | ||
| */ | ||
| void nvte_reset_cuda_state(int device); |
There was a problem hiding this comment.
Is it worth the effort to support per-device resets? I don't see why we couldn't have resources that are shared between devices, e.g. the cuDNN plan cache could have kernels for multiple devices, or the NVRTC kernel could have compiled code on CPU and per-device function pointers. It seems easier to support the desired use-case by blindly destroying everything that depends on the CUDA context.
| void nvte_reset_cuda_state(int device); | |
| void nvte_reset_cuda_state(); |
| struct DeviceState { | ||
| std::atomic<uint64_t> generation{1}; | ||
| std::set<Resource *> resources; | ||
| }; | ||
|
|
||
| namespace { | ||
| struct Registry { | ||
| std::mutex mutex; | ||
| std::map<int, DeviceState> devices; | ||
| std::atomic<uint64_t> generation{1}; | ||
| }; |
There was a problem hiding this comment.
Is there value in tracking the resources on each device?
| struct DeviceState { | |
| std::atomic<uint64_t> generation{1}; | |
| std::set<Resource *> resources; | |
| }; | |
| namespace { | |
| struct Registry { | |
| std::mutex mutex; | |
| std::map<int, DeviceState> devices; | |
| std::atomic<uint64_t> generation{1}; | |
| }; | |
| namespace { | |
| struct Registry { | |
| std::mutex mutex; | |
| std::vector<Resource *> resources; | |
| std::atomic<uint64_t> generation{1}; | |
| }; |
| } | ||
| } | ||
|
|
||
| void Resource::bind_context(int device) { |
There was a problem hiding this comment.
We could make registration automatic by putting this in the Resource constructor.
|
|
||
| struct DeviceState { | ||
| std::atomic<uint64_t> generation{1}; | ||
| std::set<Resource *> resources; |
There was a problem hiding this comment.
I wonder if there could be problems from resetting resources in a non-deterministic order. It doesn't seem like we need to support removing resources, so perhaps it would be safer to reset resources in reverse the order they were registered.
| std::set<Resource *> resources; | |
| std::vector<Resource *> resources; |
|
|
||
| // Registered objects keep stable addresses. Reset and destruction must be | ||
| // externally serialized with all TE calls. Registration is lazy and host-only. | ||
| class Resource { |
There was a problem hiding this comment.
Handling invalidation logic with a mixin class is a nice approach. As I see it, this class has the following contract:
- Class instances register themselves with a global registry with
bind_context. - The internal state of a
Resourcecan be invalidated at any time. Any time you access aResource, you need to check whether the state is invalidated and reinitialize if needed.
I wonder if we can make this more automatic:
- Class instances automatically register themselves in global registry (https://github.com/NVIDIA/TransformerEngine/pull/3779/changes#r4418682371).
- Have virtual functions for initialization and invalidation.
class Resource {
public:
Resource() {
auto &state = registry();
std::lock_guard<std::mutex> lock(state.mutex);
state.resources.push_back(this);
}
void ensure_valid() {
if (!valid_.load(std::memory_order_acquire)) {
initialize();
valid_.store(true, std::memory_order_release);
}
}
private:
friend void reset();
virtual void initialize() = 0;
virtual void invalidate() noexcept = 0;
std::atomic<bool> valid_{false};
};
void reset() {
auto &state = registry();
std::lock_guard<std::mutex> lock(state.mutex);
for (auto it = state.resources.rbegin(); it != state.resources.rend(); ++it) {
if (it->valid_.load(std::memory_order_acquire)) {
it->invalidate();
it->valid_.store(false, std::memory_order_release);
}
}
state.generate.fetch_add(1, std::memory_order_release);
}The only requirement for subclasses is to implement the virtual methods and to call ensure_valid() before accessing state.
Add explicit CUDA state lifecycle APIs to prevent reuse of cached TE resources across in-process restarts. Release resources while CUDA remains usable, or invalidate supported native resources after externally managed context loss, and recreate them lazily.
Framework cleanup preserves model tensors and FP8 numerical history. Unsupported framework recovery and unsafe communication/JIT teardown fail explicitly.
Type of change
Changes
Validation and remaining issues
Checklist:
Unchecked items require full L0/CI validation, resolution or triage of the reported failures/warnings, and review of the remaining recovery limitations. The contributing guidelines have been read; their full validation requirements are not yet satisfied