On Mon, 2026-07-06 at 09:45 +0100, Tvrtko Ursulin wrote:
On 03/07/2026 15:47, Philipp Stanner wrote:
I think it can detail which functions will now be locked; but mentioning the users would be overkill and is uncommon for API reworks.
Here I disagree quite strongly. Given the patch is making strong claims that the lockless access was added for no obvious reason, and that we have now established the lockless helper is in fact used on the submission paths, it is really required that those strong claims are backed by a concrete analysis instead of just saying "not performance critical in any way".
This is a strong case for the reversal of the burden of proof.
The entire code base of drm_sched has been designed on the computer science premise of locks being evil. That's why literally all synchronization primitives except for locks have been used where possible, including undefined behavior. The designers tried as hard as they could to avoid locks.
That is clearly proven by the fact that in all original data type definitions, the only components that were locked were always lists, since those are the structures where you really cannot avoid a lock in most cases.
The aversion to locking was so great that they designed spsc_queue, which uses at least as many as expensive instructions as a lock + list would have needed, and its correctness is not proven, nor are its behavior and rules neither documented or proven.
It's not up to the faction who wants to use correct locking and phase out UB to prove that the locklessness is bad, but to whomever added the locklessness to prove why it is good, i.e., necessary – which was not done here, neither in comments nor commit message. So the reasonable assumption is that it's simply a leftover from a flawed, broken design.
And the kernel-workflow is that things are always on-list for a while before being merged is that parties who do have concerns and who can point out problems have time to do so. Which is of course open to you: do you see a performance-regression problem with this patch, and if so, where?
Anyways: * Correct me if I'm wrong, but it would seem the only driver-usage which could see a *new* lock in its path is drm_sched_entity_error(), for which you yourself agree that it's irrelevant performance-wise. Should we still list the user's of that function? * The other relevant user path, drm_sched_job_arm() via drm_sched_entity_select_rq(), must already be called under a common driver lock for drm_sched_entity_push_job(), and _select_rq() already takes the entity lock. So any significant regression here is hyper unlikely. * The only other contender is the job pull path, which runs serially, by 1 work item at one point in time. * drm_sched_entity_kill() / _fini() are used in user context teardown path. Performance irrelevant.
I can offer to add the list above for the justification of why removing the half-undefined behavior is good.
Or what exactly would you want to see documented? "amdgpu uses drm_sched_job_arm() and now sees a lock-critical section longer by 3 instructions. etnaviv uses drm_sched_job_arm() and now…"?
P.