[Question] executeCommandLists Race Condition

[jambuttenshaw]

Hello,

I have a simple application with two threads, one will be doing simulation work and another graphics work. Both are submitting work to the graphics queue (I just want the simulation work to be performed at fixed intervals, rather than it being necessarily asynchronous).

However, I seem to be hitting what seems to be some race condition when submitting work. The application crashes with the following message:
D3D12 ERROR: ID3D12CommandQueue1::ExecuteCommandLists: Failed to execute a command list 0x000001F62CFD8C40:'Unnamed ID3D12GraphicsCommandList Object' because the command queue fence has not advanced past previous executions of the command list. [ EXECUTION ERROR #553: COMMAND_LIST_SYNC]
Introducing an EventQuery to stall the simulation thread until recording more commands (and even introducing an WaitUntilIdle) did not prevent the crash, however guarding calls to executeCommandLists with a mutex does prevent the crash.

I wanted to ask if it is known that executeCommandLists is not thread-safe and should be guarded with a mutex, or if this is unexpected behaviour and I have another issue in my application.

[apanteleev]

Looking at the code, I don't see any thread-safety features in executeCommandLists. It's easy enough to put a simple lock_guard in there, maybe with one mutex per queue, to make it safe. I think we just don't have any multi-threaded command list submission code to test that with.

[YiRanMushroom]

I was actually just about to ask a similar question myself because I've been running into some concerns with this as well. It feels like the command list submission in NVRHI is fundamentally designed as a stateful process tied to the device.

Operations like setEventQuery or the way Vulkan semaphores are configured seem to depend on global state inside the IDevice itself. While I'm not as intimately familiar with the D3D12 backend, I suspect it follows a similar pattern where the device tracks the submission context internally.

Because these submission-related calls update and rely on that global device state, they aren't truly stateless or isolated. It seems like the architecture implicitly requires you to serialize submissions on a single thread to avoid race conditions or corrupted internal states within the device, which is what I did in my application.

[chrisd-work]

I talked to @apanteleev about this yesterday. We agree ideally that we should match the level of guarantees that D3D12 and Vulkan promise in these cases. Notably, if they are not thread-safe in some situations, we do not want to add additional overhead forcing them to be thread safe.

We will spend some time looking at the documentation on both the Vulkan and D3D12 side and come back with a proposal about the degree of thread safety we should promise. If we do not meet these levels, we will create a pull request to achieve greater safety.

[jambuttenshaw]

Some additional observations from my experiences so far (I've only been working with the D3D12 backend, so there may be additional considerations to also work with Vulkan):

To progress I have just wrapped all executeCommandLists calls with a mutex, but this isn't sufficient as race conditions can still be (rarely) encountered when a worker thread submits work while the main thread is running garbage collection on device resources - both threads simultaneously access the command lists in flight structure. Perhaps some solution where each thread collects and manages its own command lists in flight, rather than in a centralized location, could offer a flexible solution with minimal critical sections?

The remaining critical section would be guarding the fence with each queue, but I can't see how this would be avoidable, unless you wanted to separate submission from synchronization and let users control when they increment fence values (but this would have consequences for tracking active command lists).

Applications can avoid large numbers of threads trying to call executeCommandLists simultaneously with a fork-join pattern to record commands and then submit on a single thread, but its still quite useful for executeCommandLists to be thread-safe in general for producer-consumer patterns with a low number of threads and submission calls.

[chrisd-work]

Thanks for the follow up.

Looking at the code the most obvious concern I had in the use case you gave was the fact that m_CommandListsToExecute would accessed from multiple threads at a time. This is something that should be fixed.

However, your example is could be worrisome.

I am trying to figure out however, is this something a user would have to solve on D3D12, or is it something specific to NVRHI? Notably, when work occurs on the GPU, that work occurs at some undefined time after executeCommandLists. This means that to garbage collect an application must use some sort of synchronization primitive. Notably the application has to know the following:

1. What was the last time time the application sent a given resource to the GPU?
2. Has the GPU completed the commands up to and including the last usage of that resource.

Then and only then can the application remove the resource. We do not what to solve this problem in NVRHI, there are a ton of ways to solve the problem, with varying tradeoffs. This is the same reason that neither Vulkan, nor D3D12 solve it for you.

You don't necessarily have to keep track of every command list. In many applications I have written I simply record when frames are done and remove resources once the last frame they were used on the GPU completes. Since each queue is a FIFO, you can simply record your own sequential counter on each queue. It does become cumbersome if a given resource is used in n queues. As though each queue is a FIFO, the queues themselves do not have to expressly synchronize with each other. This, however, is the exception, rather than the rule for applications, and resources This does show why we wouldn't want to solve this at the NVRHI level.

If I have missed something and this is something D3D12 takes care of for you, but NVRHI does not let me know. Our general design goal here is to create a lightweight cross API wrapper for D3D12 and Vulkan, not to create a higher level API.

[chrisd-work]

I am pretty new to this code base I am going to look deeper, I now see what you are talking about with the Device::runGarbageCollection

[manuelk]

AFAIK, we haven't removed d3d11 from the supported back-ends yet: client-code compatibility has made some impositions on NVRHI's API. Maybe it is time to retire the d3d11 back-end ?

With that said, i vaguely remember implementing multiple async command lists for a frame-pacing project and running into idiosyncrasies between D3D12 enhanced barriers and Vk's equivalent. At the time, it was enough of a headache that i decided to punt on implementing the Vk NVRHI back-end altogether : IIRC this problem still exists today.

[chrisd-work]

Per @manuelk 's comment, I have added the following issue to propose removing Direct3D 11 support #115

[jambuttenshaw]

NVRHI already does more than D3D12/Vk would do for you in terms of managing resource lifetimes, which I think is a great feature and one of the most attractive about NVRHI, and it would be superb to extend this to multithreaded applications also.

The core of the issue is that command list submission and lifetime management are tied together (and also tied to the core application loop in the case of Donut - where garbage collection is initiated inside the DeviceManager rather than application code). I think this issue could be resolved with finer-grained control on the application side.

For example, a possible implementation could be: executeCommandLists would, instead of adding command lists to a collection within the device, it would just make the underlying API call, atomically increment a fence associated with the queue, and return an EventQueryHandle which can be used to track when the GPU work execution completes. Already executeCommandLists returns the fence value associated with the submission - extending this to return a full EventQueryHandle would be much more practical - but would need some thought as how to handle the allocation of the IEventQuery objects, since they are treated as resources that may not be trivial to allocate (even though in practice they are pretty much POD structures).

To continue to enable automatic command list lifetime tracking, the command list and associated event query could be moved into an ICommandListLifetimeManager, which simply stores command lists in flight and their associated event queries, and applications can tick the manager frequently to poll the event queries and release the command lists when possible (similar to how runGarbageCollection behaves currently), and then each thread that submits work can have its own ICommandListLifetimeManager and tick it as frequently as required (e.g. every frame / every simulation step / etc).

Potentially having this as an opt-in feature would make this a smoother change from the existing behaviour (i.e., the IDevice owns its own ICommandListLifetimeManager by default and clients can declare when they would like the device to NOT use its own lifetime management). This could be achieved by an optional argument to executeCommandLists specifying an ICommandListLifetimeManager to use?

I'm not sure how this would map to D3D11, I'm not sure if the RHI can hide the fact that the underlying API just doesn't support the multithreading capabilities of the modern APIs.