On 06/07/2026 10:42, Philipp Stanner wrote:
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.
I did not want us to discourse into side philosophical arguments about
how and why the code base got to where it is. The story at hand is much
simpler and narrower so excuse me for skipping the majority of the below.
Put in a different wording, what I was trying to express is this: Lets
not write a poor commit message because the original one was poor. Aka
two wrongs do not make a right.
Also, I am not putting a burden of proof on you, in fact, I offered to
test your series.
As to your closing question to what I suggest commit message needs to
add, that is simple, and yes, the first bullet point you list is what I
have already asked for in one of the previous replies. So say something
along the lines of:
"""
drm_sched_entity_error(), which is significantly used from various
stages of the amdgpu job submit path, either directly or via
amdgpu_vm_generation(), is changed from lockless to taking the
entity->lock. As performance was not listed as a reason the lockless
approach was chosen in the above referenced commit, although it is
suspected that might have been the motivation, it is now thought that
the new lock cycles to those paths will not add any measurable overhead.
For other drivers no new lock cycles are added to the submit path, given
drm_sched_entity_select_rq() via the job arm path already bails out
early due all drivers apart from AMD only passing a single scheduler
list to the entity.
"""
Bonus point if you can spend the time to count how many extra
lock-unlock cycles it is adding between it's CS submit, prepare job and
run job entry points.
For me, that is not the burden of proof but some minimum standard of a
commit message which shows some due diligence was done.
Regards,
Tvrtko
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.
