On Tue, Sep 12, 2023 at 02:18:18PM +0200, Boris Brezillon wrote: > On Tue, 12 Sep 2023 12:46:26 +0200 > Danilo Krummrich <dakr@xxxxxxxxxx> wrote: > > > > I'm a bit worried that leaving this single vs multi-threaded wq > > > decision to drivers is going to cause unnecessary pain, because what > > > was previously a granted in term of run/cleanup execution order (thanks > > > to the kthread+static-drm_sched_main-workflow approach) is now subject > > > to the wq ordering guarantees, which depend on the wq type picked by > > > the driver. > > > > Not sure if this ends up to be much different. The only thing I could think of > > is that IIRC with the kthread implementation cleanup was always preferred over > > run. > > Given the sequence in drm_sched_main(), I'd say that cleanup and run > operations are naturally interleaved when both are available, but I > might be wrong. >From drm_sched_main(): wait_event_interruptible(sched->wake_up_worker, (cleanup_job = drm_sched_get_cleanup_job(sched)) || (!drm_sched_blocked(sched) && (entity = drm_sched_select_entity(sched))) || kthread_should_stop()); if (cleanup_job) sched->ops->free_job(cleanup_job); if (!entity) continue; If cleanup_job is not NULL the rest shouldn't be evaluated I guess. Hence entity would be NULL and we'd loop until there are no more cleanup_jobs if I don't miss anything here. > > > With a single threaded wq this should be a bit more balanced. > > With a single threaded wq, it's less clear, because each work > reschedules itself for further processing, but it's likely to be more > or less interleaved. Anyway, I'm not too worried about cleanup taking > precedence on run or the other way around, because the limited amount > of HW slots (size of the ring-buffer) will regulate that. Yeah, that's what I meant, with to work items rescheduling themselves it starts to be interleaved. Which I'm not worried about as well. > > > > > With a multi-threaded wq it's still the same, but run and cleanup can run > > concurrently, > > What I'm worried about is that ^. I'm not saying it's fundamentally > unsafe, but I'm saying drm_sched hasn't been designed with this > concurrency in mind, and I fear we'll face subtle bugs if we go from > kthread to multi-threaded-wq+run-and-cleanup-split-in-2-work-items. > Yeah, so what we get with that is that job_run() of job A and job_free() of job B can run in parallel. Unless drivers do weird things there, I'm not seeing an issue with that as well at a first glance. > > which has the nice side effect that free_job() gets out of the > > fence signaling path. At least as long as the workqueue has max_active > 1. > > Oh, yeah, I don't deny using a multi-threaded workqueue has some > benefits, just saying it might be trickier than it sounds. > > > Which is one reason why I'm using a multi-threaded wq in Nouveau. > > Note that I'm using a multi-threaded workqueue internally at the moment > to deal with all sort of interactions with the FW (Mali HW only has a > limited amount of scheduling slots, and we need to rotate entities > having jobs to execute so every one gets a chance to run on the GPU), > but this has been designed this way from the ground up, unlike > drm_sched_main() operations, which were mostly thought as a fixed > sequential set of operations. That's not to say it's impossible to get > right, but I fear we'll face weird/unexpected behavior if we go from > completely-serialized to multi-threaded-with-pseudo-random-processing > order. >From a per job perspective it's still all sequential and besides fence dependencies, which are still resolved, I don't see where jobs could have cross dependencies that make this racy. But agree that it's probably worth to think through it a bit more. > > > > > That latter seems a bit subtile, we probably need to document this aspect of > > under which conditions free_job() is or is not within the fence signaling path. > > Well, I'm not even sure it can be clearly defined when the driver is > using the submit_wq for its own work items (which can be done since we > pass an optional submit_wq when calling drm_sched_init()). Sure, having > max_active >= 2 should be enough to guarantee that the free_job work > won't block the run_job one when these are the 2 only works being > queued, but what if you have many other work items being queued by the > driver to this wq, and some of those try to acquire resv locks? Could > this prevent execution of the run_job() callback, thus preventing > signaling of fences? I'm genuinely asking, don't know enough about the > cmwq implementation to tell what's happening when work items are > blocked (might be that the worker pool is extended to unblock the > situation). Yes, I think so. If max_active would be 2 and you have two jobs running on this workqueue already waiting on allocations then the 3rd job signaling the fence the allocation is blocked by would be stuck and we'd have a deadlock I guess. But that's where I start to see the driver being responsible not to pass a workqueue to the driver where it queues up other work, either at all, or that interferes with fence signaling paths. So, I guess the message here would be something like: free_job() must be considered to be in the fence signaling path, unless the submit_wq is a multi-threaded workqueue with max_active > 1 *dedicated* to the DRM scheduler. Otherwise it's the drivers full resposibility to make sure it doesn't violate the rules. > > Anyway, documenting when free_job() is in the dma signalling path should > be doable (single-threaded wq), but at this point, are we not better > off considering anything called from the submit_wq as being part of the > dma signalling path, so we can accommodate with both cases. And if > there is cleanup processing that require taking dma_resv locks, I'd be > tempted to queue that to a driver-specific wq (which is what I'm doing > right now), just to be safe. > It's not only the dma-resv lock, it's any lock under which allocations may be performed.
