[PATCH] Documentation: drm: Add entry for removing spsc_queue to TODO list

[PATCH] Documentation: drm: Add entry for removing spsc_queue to TODO list

[Philipp Stanner]

drm_sched contains a lockless queue (spsc_queue) that seems to be
useless and potentially unsound.

Add a TODO list entry for replacing spsc_queue with a locked list.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 Documentation/gpu/todo.rst | 41 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index cdddf8db35f5..87e082b0eb48 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -948,6 +948,47 @@ Contact: Philipp Stanner <phasta@kernel.org>
 
 Level: Intermediate
 
+Replace the lockless queue with a locked list
+---------------------------------------------
+
+drm_sched is the only user in the entire kernel of a special lockless queue, the
+spsc_queue. This queue utilizes:
+
+- preempt_disable()
+- atomic instructions
+- memory barriers
+- ACCESS_ONCE()
+
+whereas a conventional spinlock utilizes:
+
+- preempt_disable()
+- 1 atomic instruction for taking / releasing the lock
+- memory barriers
+
+Moreover, drm_sched_entity_push_job(), the only user of spsc_queue_push(), has
+to take a lock in some situations anyways and calls to it are often serialized
+with a driver lock.
+
+It is, thus, highly questionable whether the lockless queue grants any advantage
+at all. Considering that its internals are not well documented and its correctness
+is not formally proven, it seems desirable to replace the queue with a mere list
+or hlist that is protected by a spinlock.
+
+Tasks:
+
+- Replace the spsc_queue in drm/sched (and those who might access the scheduler's
+  internal queue) with a spinlock + (h)list.
+- Ideally, check with some micro benchmarks and real world tests (preferably
+  with amdgpu) for relevant performance regressions.
+- Remove the spsc_queue from the kernel altogether.
+
+Contact:
+
+- Philipp Stanner <phasta@kernel.org>
+- Christian König <christian.koenig@amd.com>
+
+Level: Beginner
+
 Outside DRM
 ===========
 
-- 
2.54.0

Re: [PATCH] Documentation: drm: Add entry for removing spsc_queue to TODO list

[Christian König]

On 5/29/26 23:52, Philipp Stanner wrote:
> drm_sched contains a lockless queue (spsc_queue) that seems to be
> useless and potentially unsound.
> 
> Add a TODO list entry for replacing spsc_queue with a locked list.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  Documentation/gpu/todo.rst | 41 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index cdddf8db35f5..87e082b0eb48 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -948,6 +948,47 @@ Contact: Philipp Stanner <phasta@kernel.org>
>  
>  Level: Intermediate
>  
> +Replace the lockless queue with a locked list
> +---------------------------------------------
> +
> +drm_sched is the only user in the entire kernel of a special lockless queue, the
> +spsc_queue. This queue utilizes:
> +
> +- preempt_disable()
> +- atomic instructions
> +- memory barriers
> +- ACCESS_ONCE()
> +
> +whereas a conventional spinlock utilizes:
> +
> +- preempt_disable()
> +- 1 atomic instruction for taking / releasing the lock
> +- memory barriers
> +
> +Moreover, drm_sched_entity_push_job(), the only user of spsc_queue_push(), has
> +to take a lock in some situations anyways and calls to it are often serialized
> +with a driver lock.
> +
> +It is, thus, highly questionable whether the lockless queue grants any advantage
> +at all. Considering that its internals are not well documented and its correctness
> +is not formally proven, it seems desirable to replace the queue with a mere list
> +or hlist that is protected by a spinlock.
> +
> +Tasks:
> +
> +- Replace the spsc_queue in drm/sched (and those who might access the scheduler's
> +  internal queue) with a spinlock + (h)list.
> +- Ideally, check with some micro benchmarks and real world tests (preferably
> +  with amdgpu) for relevant performance regressions.
> +- Remove the spsc_queue from the kernel altogether.
> +
> +Contact:
> +
> +- Philipp Stanner <phasta@kernel.org>
> +- Christian König <christian.koenig@amd.com>
> +
> +Level: Beginner
> +
>  Outside DRM
>  ===========
>  

Re: [PATCH] Documentation: drm: Add entry for removing spsc_queue to TODO list

[Philipp Stanner]

On Mon, 2026-06-01 at 10:30 +0200, Christian König wrote:
> 
> 
> On 5/29/26 23:52, Philipp Stanner wrote:
> > drm_sched contains a lockless queue (spsc_queue) that seems to be
> > useless and potentially unsound.
> > 
> > Add a TODO list entry for replacing spsc_queue with a locked list.
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

Pushed to drm-misc-next

Thx
P.

> 
> > ---
> >  Documentation/gpu/todo.rst | 41 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index cdddf8db35f5..87e082b0eb48 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -948,6 +948,47 @@ Contact: Philipp Stanner <phasta@kernel.org>
> >  
> >  Level: Intermediate
> >  
> > +Replace the lockless queue with a locked list
> > +---------------------------------------------
> > +
> > +drm_sched is the only user in the entire kernel of a special lockless queue, the
> > +spsc_queue. This queue utilizes:
> > +
> > +- preempt_disable()
> > +- atomic instructions
> > +- memory barriers
> > +- ACCESS_ONCE()
> > +
> > +whereas a conventional spinlock utilizes:
> > +
> > +- preempt_disable()
> > +- 1 atomic instruction for taking / releasing the lock
> > +- memory barriers
> > +
> > +Moreover, drm_sched_entity_push_job(), the only user of spsc_queue_push(), has
> > +to take a lock in some situations anyways and calls to it are often serialized
> > +with a driver lock.
> > +
> > +It is, thus, highly questionable whether the lockless queue grants any advantage
> > +at all. Considering that its internals are not well documented and its correctness
> > +is not formally proven, it seems desirable to replace the queue with a mere list
> > +or hlist that is protected by a spinlock.
> > +
> > +Tasks:
> > +
> > +- Replace the spsc_queue in drm/sched (and those who might access the scheduler's
> > +  internal queue) with a spinlock + (h)list.
> > +- Ideally, check with some micro benchmarks and real world tests (preferably
> > +  with amdgpu) for relevant performance regressions.
> > +- Remove the spsc_queue from the kernel altogether.
> > +
> > +Contact:
> > +
> > +- Philipp Stanner <phasta@kernel.org>
> > +- Christian König <christian.koenig@amd.com>
> > +
> > +Level: Beginner
> > +
> >  Outside DRM
> >  ===========
> >  

Claude review: Documentation: drm: Add entry for removing spsc_queue to TODO list

[Claude Code Review Bot]

Overall Series Review

Subject: Documentation: drm: Add entry for removing spsc_queue to TODO list
Author: Philipp Stanner <phasta@kernel.org>
Patches: 3
Reviewed: 2026-06-04T15:59:15.697169

---

This is a single documentation-only patch adding a TODO entry to `Documentation/gpu/todo.rst` for replacing the lockless `spsc_queue` in drm_sched with a spinlock-protected list. The motivation is sound — spsc_queue is indeed only used by drm_sched in the entire kernel, and replacing it with a simpler locked list would reduce maintenance burden of a niche lockless data structure. The TODO entry is well-structured and clearly describes the tasks.

However, there is a factual inaccuracy in the patch text that should be corrected before merging.

---
Generated by Claude Code Patch Reviewer

Claude review: Documentation: drm: Add entry for removing spsc_queue to TODO list

[Claude Code Review Bot]

Patch Review

**Factual error — `ACCESS_ONCE()` reference is wrong:**

The patch states:

```
+This queue utilizes:
+
+- preempt_disable()
+- atomic instructions
+- memory barriers
+- ACCESS_ONCE()
```

`ACCESS_ONCE()` was removed from the kernel years ago and replaced with `READ_ONCE()`/`WRITE_ONCE()`. The current `spsc_queue.h` uses `READ_ONCE()` and `WRITE_ONCE()` exclusively. This should be corrected to `READ_ONCE() / WRITE_ONCE()`.

**Minor: "has to take a lock in some situations" claim is misleading:**

The patch says:

```
+Moreover, drm_sched_entity_push_job(), the only user of spsc_queue_push(), has
+to take a lock in some situations anyways and calls to it are often serialized
+with a driver lock.
```

Looking at the current code, `drm_sched_entity_push_job()` itself does not take any lock internally. Rather, its documentation states that callers should hold a "common lock" externally to guarantee ordering with `drm_sched_job_arm()`. This is an important distinction — the function relies on external serialization, but doesn't itself take any lock "in some situations." The wording could be made more precise to say something like "calls to it are typically externally serialized by a driver lock" to better reflect reality.

**Level classification:**

The task is marked as "Beginner" level. This seems reasonable for the replacement itself, though verifying correctness with benchmarks (especially on amdgpu) may push it toward Intermediate. The existing TODO entry immediately above it (about locking in `drm_sched_rq`) is marked Intermediate for similar work, so there's a minor inconsistency, but this is a judgment call.

**Structural nit — contact formatting:**

The existing TODO entries in the file use a single-line `Contact:` format like:

```
Contact: Philipp Stanner <phasta@kernel.org>
```

This patch uses a multi-line list format:

```
+Contact:
+
+- Philipp Stanner <phasta@kernel.org>
+- Christian König <christian.koenig@amd.com>
```

The list format is actually fine since there are two contacts, but worth noting the slight style difference. No change needed.

**Summary:** The patch is a reasonable TODO addition. The main actionable item is correcting the `ACCESS_ONCE()` reference to `READ_ONCE() / WRITE_ONCE()`, and optionally clarifying the locking claim about `drm_sched_entity_push_job()`.

---
Generated by Claude Code Patch Reviewer