drm/sched: Switch the default policy to "fair"
Subsystems (3)
PATCH 1/1 drm/sched: Switch the default policy to "fair"
After the last known real fair policy bug was fixed in commit:2ab510e63197 ("drm/sched: Fix virtual runtime race")
And two theoretical ones in commits:cda6f72545f2 ("drm/sched: Fix out of bounds entity priority check" 75709a35da45 ("drm/sched: Adjust run-queue when changing priority at runtime")
Lets switch the default policy to fair.
---
If everything goes well in the next release we can then continue removing the obsolete policies and simplifying the run-queue management.
--- drivers/gpu/drm/scheduler/sched_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/scheduler/sched_main.c +2 -2
@@ -83,13 +83,13 @@#define CREATE_TRACE_POINTS #include "gpu_scheduler_trace.h" -int drm_sched_policy =DRM_SCHED_POLICY_FIFO ; +int drm_sched_policy =DRM_SCHED_POLICY_FAIR ;/** * DOC: sched_policy (int) * Used to override default entities scheduling policy in a run queue. */ -MODULE_PARM_DESC(sched_policy, "Specify the scheduling policy for entities on a run-queue, " __stringify(DRM_SCHED_POLICY_RR) " = Round Robin, " __stringify(DRM_SCHED_POLICY_FIFO) " = FIFO (default), " __stringify(DRM_SCHED_POLICY_FAIR) " = Fair (experimental)."); +MODULE_PARM_DESC(sched_policy, "Specify the scheduling policy for entities on a run-queue, " __stringify(DRM_SCHED_POLICY_RR) " = Round Robin, " __stringify(DRM_SCHED_POLICY_FIFO) " = FIFO, " __stringify(DRM_SCHED_POLICY_FAIR) " = Fair (default).");module_param_named (sched_policy, drm_sched_policy, int,0444 );static u32 drm_sched_available_credits (struct drm_gpu_scheduler *sched)
Signature · 1 line
2.56.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found no new issues in this patch, but noticed 1 related pre-existing issue it does not introduce: - [High] drm/sched: ZERO_SIZE_PTR dereference in…
Thank you for your contribution! Sashiko AI review found no new issues in this patch, but noticed 1 related pre-existing issue it does not introduce:
- [High] drm/sched: ZERO_SIZE_PTR dereference in drm_sched_init() [1] --
commit 50f60e38bc825925735e0e6bb31a4a450af5bbc2
Author: Tvrtko Ursulin <[email protected]>
drm/sched: Switch the default policy to "fair"
This commit changes the default DRM GPU scheduler policy from FIFO to fair and updates the sched_policy module parameter description.
drivers/gpu/drm/scheduler/sched_main.c line 86
Quoted 10 lines
drivers/gpu/drm/scheduler/sched_main.c +1
-1@@ -83,13 +83,13 @@#define CREATE_TRACE_POINTS #include "gpu_scheduler_trace.h" -int drm_sched_policy =DRM_SCHED_POLICY_FIFO ; +int drm_sched_policy =DRM_SCHED_POLICY_FAIR ;
[Severity: High]
This problem wasn't introduced by this patch, but can drm_sched_init() dereference ZERO_SIZE_PTR on sched->sched_rq [1] when args->num_rqs is 0 under DRM_SCHED_POLICY_FAIR?
drivers/gpu/drm/scheduler/sched_main.c:drm_sched_init() {
...
sched->num_rqs = drm_sched_policy != DRM_SCHED_POLICY_FAIR ?
args->num_rqs : 1;
sched->sched_rq = kzalloc_objs(*sched->sched_rq, args->num_rqs);
if (!sched->sched_rq)
goto Out_check_own;
for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
sched->sched_rq[i] = kzalloc_obj(*sched->sched_rq[i]);
...
}When args->num_rqs is 0, it passes the upper-bound check against DRM_SCHED_PRIORITY_COUNT, and kzalloc_objs() returns ZERO_SIZE_PTR while sched->num_rqs is overridden to 1 for the fair policy, causing the loop to dereference sched->sched_rq[0]. Should kzalloc_objs() use sched->num_rqs instead of args->num_rqs?
[1] https://sashiko.dev/#/bug/linux-5fb1fd33-7544-4ca1-bec8-2b20739ac0dd
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 8 lines · Tvrtko Ursulin
After the last known real fair policy bug was fixed in commit:
2ab510e63197("drm/sched: Fix virtual runtime race")And two theoretical ones in commits:
cda6f72545f2("drm/sched: Fix out of bounds entity priority check"75709a35da45("drm/sched: Adjust run-queue when changing priority at runtime")Lets switch the default policy to fair.
We shall, but first let's get the spsc_queue series in. Considering that the last time we blew up the kernel with CFS was – ultimately – caused by the scheduler's lack of locking, getting rid of a magic lockless queue involved in all entities seems the safest path available.
Christian's recent comments [1] indicate that the complicated AMD fix might be dropped altogether, so we can be positive that spsc can be merged relatively quickly.
Philipp
Quoted 16 lines · Philipp Stanner
Quoted 8 lines · Tvrtko Ursulin
After the last known real fair policy bug was fixed in commit:
2ab510e63197("drm/sched: Fix virtual runtime race")And two theoretical ones in commits:
cda6f72545f2("drm/sched: Fix out of bounds entity priority check"75709a35da45("drm/sched: Adjust run-queue when changing priority at runtime")Lets switch the default policy to fair.
We shall, but first let's get the
spsc_queueseries in. Considering that the last time we blew up the kernel with CFS was – ultimately – caused by the scheduler's lack of locking, getting rid of a magic lockless queue involved in all entities seems the safest path available.
Quoted 3 lines
Christian's recent comments [1] indicate that the complicated AMD fix might be dropped altogether, so we can be positive that spsc can be merged relatively quickly.
Okay, we are keeping an eye on that and we will see.
On the topic of the fair policy, with the latest SteamOS update I pulled yesterday, and kwin_wayland moving to high priority gfx context, I've noticed job runtime accounting is a bit off so I will see if I can improve that in the meantime. Problem is when there is no preemption and two gfx rings share the same hardware it overaccounts since wall time != gpu time. I've sent one trivial and unrelated job runtime accounting fix
but for the shared pipes and no preemption it is still in
brainstorming stage.
Regards,
Tvrtko
Quoted 5 lines
Philipp