* [PATCH 1/8] drm/sched: Add locking to drm_sched_entity_modify_sched
[not found] <20240909171937.51550-1-tursulin@igalia.com>
@ 2024-09-09 17:19 ` Tvrtko Ursulin
2024-09-10 7:43 ` Christian König
2024-09-09 17:19 ` [PATCH 2/8] drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job Tvrtko Ursulin
2024-09-09 17:19 ` [PATCH 3/8] drm/sched: Always increment correct scheduler score Tvrtko Ursulin
2 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2024-09-09 17:19 UTC (permalink / raw)
To: amd-gfx, dri-devel
Cc: Tvrtko Ursulin, Christian König, Alex Deucher, Luben Tuikov,
Matthew Brost, David Airlie, Daniel Vetter, Philipp Stanner,
stable
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Without the locking amdgpu currently can race between
amdgpu_ctx_set_entity_priority() (via drm_sched_entity_modify_sched()) and
drm_sched_job_arm(), leading to the latter accesing potentially
inconsitent entity->sched_list and entity->num_sched_list pair.
v2:
* Improve commit message. (Philipp)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: b37aced31eb0 ("drm/scheduler: implement a function to modify sched list")
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Luben Tuikov <ltuikov89@gmail.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: Philipp Stanner <pstanner@redhat.com>
Cc: <stable@vger.kernel.org> # v5.7+
---
drivers/gpu/drm/scheduler/sched_entity.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 58c8161289fe..ae8be30472cd 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -133,8 +133,10 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
{
WARN_ON(!num_sched_list || !sched_list);
+ spin_lock(&entity->rq_lock);
entity->sched_list = sched_list;
entity->num_sched_list = num_sched_list;
+ spin_unlock(&entity->rq_lock);
}
EXPORT_SYMBOL(drm_sched_entity_modify_sched);
--
2.46.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/8] drm/sched: Add locking to drm_sched_entity_modify_sched
2024-09-09 17:19 ` [PATCH 1/8] drm/sched: Add locking to drm_sched_entity_modify_sched Tvrtko Ursulin
@ 2024-09-10 7:43 ` Christian König
0 siblings, 0 replies; 7+ messages in thread
From: Christian König @ 2024-09-10 7:43 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx, dri-devel
Cc: Tvrtko Ursulin, Christian König, Alex Deucher, Luben Tuikov,
Matthew Brost, David Airlie, Daniel Vetter, Philipp Stanner,
stable
Am 09.09.24 um 19:19 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Without the locking amdgpu currently can race between
> amdgpu_ctx_set_entity_priority() (via drm_sched_entity_modify_sched()) and
> drm_sched_job_arm(), leading to the latter accesing potentially
> inconsitent entity->sched_list and entity->num_sched_list pair.
>
> v2:
> * Improve commit message. (Philipp)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Fixes: b37aced31eb0 ("drm/scheduler: implement a function to modify sched list")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Luben Tuikov <ltuikov89@gmail.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Philipp Stanner <pstanner@redhat.com>
> Cc: <stable@vger.kernel.org> # v5.7+
Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/scheduler/sched_entity.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 58c8161289fe..ae8be30472cd 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -133,8 +133,10 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> {
> WARN_ON(!num_sched_list || !sched_list);
>
> + spin_lock(&entity->rq_lock);
> entity->sched_list = sched_list;
> entity->num_sched_list = num_sched_list;
> + spin_unlock(&entity->rq_lock);
> }
> EXPORT_SYMBOL(drm_sched_entity_modify_sched);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/8] drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job
[not found] <20240909171937.51550-1-tursulin@igalia.com>
2024-09-09 17:19 ` [PATCH 1/8] drm/sched: Add locking to drm_sched_entity_modify_sched Tvrtko Ursulin
@ 2024-09-09 17:19 ` Tvrtko Ursulin
2024-09-10 7:42 ` Christian König
2024-09-09 17:19 ` [PATCH 3/8] drm/sched: Always increment correct scheduler score Tvrtko Ursulin
2 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2024-09-09 17:19 UTC (permalink / raw)
To: amd-gfx, dri-devel
Cc: Tvrtko Ursulin, Christian König, Alex Deucher, Luben Tuikov,
Matthew Brost, David Airlie, Daniel Vetter, Philipp Stanner,
stable
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Since drm_sched_entity_modify_sched() can modify the entities run queue,
lets make sure to only dereference the pointer once so both adding and
waking up are guaranteed to be consistent.
Alternative of moving the spin_unlock to after the wake up would for now
be more problematic since the same lock is taken inside
drm_sched_rq_update_fifo().
v2:
* Improve commit message. (Philipp)
* Cache the scheduler pointer directly. (Christian)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: b37aced31eb0 ("drm/scheduler: implement a function to modify sched list")
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Luben Tuikov <ltuikov89@gmail.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Philipp Stanner <pstanner@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.7+
---
drivers/gpu/drm/scheduler/sched_entity.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index ae8be30472cd..76e422548d40 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -599,6 +599,9 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
/* first job wakes up scheduler */
if (first) {
+ struct drm_gpu_scheduler *sched;
+ struct drm_sched_rq *rq;
+
/* Add the entity to the run queue */
spin_lock(&entity->rq_lock);
if (entity->stopped) {
@@ -608,13 +611,16 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
return;
}
- drm_sched_rq_add_entity(entity->rq, entity);
+ rq = entity->rq;
+ sched = rq->sched;
+
+ drm_sched_rq_add_entity(rq, entity);
spin_unlock(&entity->rq_lock);
if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
drm_sched_rq_update_fifo(entity, submit_ts);
- drm_sched_wakeup(entity->rq->sched, entity);
+ drm_sched_wakeup(sched, entity);
}
}
EXPORT_SYMBOL(drm_sched_entity_push_job);
--
2.46.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/8] drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job
2024-09-09 17:19 ` [PATCH 2/8] drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job Tvrtko Ursulin
@ 2024-09-10 7:42 ` Christian König
0 siblings, 0 replies; 7+ messages in thread
From: Christian König @ 2024-09-10 7:42 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx, dri-devel
Cc: Tvrtko Ursulin, Alex Deucher, Luben Tuikov, Matthew Brost,
David Airlie, Daniel Vetter, Philipp Stanner, stable
Am 09.09.24 um 19:19 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Since drm_sched_entity_modify_sched() can modify the entities run queue,
> lets make sure to only dereference the pointer once so both adding and
> waking up are guaranteed to be consistent.
>
> Alternative of moving the spin_unlock to after the wake up would for now
> be more problematic since the same lock is taken inside
> drm_sched_rq_update_fifo().
>
> v2:
> * Improve commit message. (Philipp)
> * Cache the scheduler pointer directly. (Christian)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Fixes: b37aced31eb0 ("drm/scheduler: implement a function to modify sched list")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Luben Tuikov <ltuikov89@gmail.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Philipp Stanner <pstanner@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.7+
> ---
> drivers/gpu/drm/scheduler/sched_entity.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index ae8be30472cd..76e422548d40 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -599,6 +599,9 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>
> /* first job wakes up scheduler */
> if (first) {
> + struct drm_gpu_scheduler *sched;
> + struct drm_sched_rq *rq;
> +
> /* Add the entity to the run queue */
> spin_lock(&entity->rq_lock);
> if (entity->stopped) {
> @@ -608,13 +611,16 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
> return;
> }
>
> - drm_sched_rq_add_entity(entity->rq, entity);
> + rq = entity->rq;
> + sched = rq->sched;
The rq variable is now not needed any more except for getting the
scheduler, isn't it? If yes I would ditch that as well.
Either way Reviewed-by: Christian König <christian.koenig@amd.com> to
this patch.
Regards,
Christian.
> +
> + drm_sched_rq_add_entity(rq, entity);
> spin_unlock(&entity->rq_lock);
>
> if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> drm_sched_rq_update_fifo(entity, submit_ts);
>
> - drm_sched_wakeup(entity->rq->sched, entity);
> + drm_sched_wakeup(sched, entity);
> }
> }
> EXPORT_SYMBOL(drm_sched_entity_push_job);
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/8] drm/sched: Always increment correct scheduler score
[not found] <20240909171937.51550-1-tursulin@igalia.com>
2024-09-09 17:19 ` [PATCH 1/8] drm/sched: Add locking to drm_sched_entity_modify_sched Tvrtko Ursulin
2024-09-09 17:19 ` [PATCH 2/8] drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job Tvrtko Ursulin
@ 2024-09-09 17:19 ` Tvrtko Ursulin
2 siblings, 0 replies; 7+ messages in thread
From: Tvrtko Ursulin @ 2024-09-09 17:19 UTC (permalink / raw)
To: amd-gfx, dri-devel
Cc: Tvrtko Ursulin, Nirmoy Das, Christian König, Luben Tuikov,
Matthew Brost, David Airlie, Daniel Vetter, stable, Nirmoy Das
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Entities run queue can change during drm_sched_entity_push_job() so make
sure to update the score consistently.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: d41a39dda140 ("drm/scheduler: improve job distribution with multiple queues")
Cc: Nirmoy Das <nirmoy.das@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Luben Tuikov <ltuikov89@gmail.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.9+
Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
---
drivers/gpu/drm/scheduler/sched_entity.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 76e422548d40..6645a8524699 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -586,7 +586,6 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
ktime_t submit_ts;
trace_drm_sched_job(sched_job, entity);
- atomic_inc(entity->rq->sched->score);
WRITE_ONCE(entity->last_user, current->group_leader);
/*
@@ -614,6 +613,7 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
rq = entity->rq;
sched = rq->sched;
+ atomic_inc(sched->score);
drm_sched_rq_add_entity(rq, entity);
spin_unlock(&entity->rq_lock);
--
2.46.0
^ permalink raw reply related [flat|nested] 7+ messages in thread