Re: [PATCH 1/2] drm/v3d: Disable preemption while updating GPU stats
Hi Tvrtko,
On 8/13/24 07:25, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
We forgot to disable preemption around the write_seqcount_begin/end() pair
while updating GPU stats:
[ ] WARNING: CPU: 2 PID: 12 at include/linux/seqlock.h:221
__seqprop_assert.isra.0+0x128/0x150 [v3d]
[ ] Workqueue: v3d_bin drm_sched_run_job_work [gpu_sched]
<...snip...>
[ ] Call trace:
[ ] __seqprop_assert.isra.0+0x128/0x150 [v3d]
[ ] v3d_job_start_stats.isra.0+0x90/0x218 [v3d]
[ ] v3d_bin_job_run+0x23c/0x388 [v3d]
[ ] drm_sched_run_job_work+0x520/0x6d0 [gpu_sched]
[ ] process_one_work+0x62c/0xb48
[ ] worker_thread+0x468/0x5b0
[ ] kthread+0x1c4/0x1e0
[ ] ret_from_fork+0x10/0x20
Fix it.
Signed-off-by: Tvrtko Ursulin
Fixes: 6abe93b621ab ("drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt
handler")
Cc: Maíra Canal
Cc: # v6.10+
Acked-by: Maíra Canal
I just applied this patch to drm-misc-fixes. I'll wait for drm-misc-
fixes to be backported to drm-misc-next before applying the second patch
to drm-misc-next.
Thanks for your contribution!
Best Regards,
- Maíra
---
drivers/gpu/drm/v3d/v3d_sched.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 42d4f4a2dba2..cc2e5a89467b 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -136,6 +136,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue
queue)
struct v3d_stats *local_stats = &file->stats[queue];
u64 now = local_clock();
+ preempt_disable();
+
write_seqcount_begin(&local_stats->lock);
local_stats->start_ns = now;
write_seqcount_end(&local_stats->lock);
@@ -143,6 +145,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue
queue)
write_seqcount_begin(&global_stats->lock);
global_stats->start_ns = now;
write_seqcount_end(&global_stats->lock);
+
+ preempt_enable();
}
static void
@@ -164,8 +168,10 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue
queue)
struct v3d_stats *local_stats = &file->stats[queue];
u64 now = local_clock();
+ preempt_disable();
v3d_stats_update(local_stats, now);
v3d_stats_update(global_stats, now);
+ preempt_enable();
}
static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
Re: [PATCH 1/2] drm/v3d: Disable preemption while updating GPU stats
Hi Tvrtko,
On 8/12/24 06:12, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
We forgot to disable preemption around the write_seqcount_begin/end() pair
while updating GPU stats:
[ ] WARNING: CPU: 2 PID: 12 at include/linux/seqlock.h:221
__seqprop_assert.isra.0+0x128/0x150 [v3d]
[ ] Workqueue: v3d_bin drm_sched_run_job_work [gpu_sched]
<...snip...>
[ ] Call trace:
[ ] __seqprop_assert.isra.0+0x128/0x150 [v3d]
[ ] v3d_job_start_stats.isra.0+0x90/0x218 [v3d]
[ ] v3d_bin_job_run+0x23c/0x388 [v3d]
[ ] drm_sched_run_job_work+0x520/0x6d0 [gpu_sched]
[ ] process_one_work+0x62c/0xb48
[ ] worker_thread+0x468/0x5b0
[ ] kthread+0x1c4/0x1e0
[ ] ret_from_fork+0x10/0x20
Fix it.
Signed-off-by: Tvrtko Ursulin
Fixes: 6abe93b621ab ("drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt
handler")
Cc: Maíra Canal
Cc: # v6.10+
---
drivers/gpu/drm/v3d/v3d_sched.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 42d4f4a2dba2..cc2e5a89467b 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -136,6 +136,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue
queue)
struct v3d_stats *local_stats = &file->stats[queue];
u64 now = local_clock();
+ preempt_disable();
+
write_seqcount_begin(&local_stats->lock);
local_stats->start_ns = now;
write_seqcount_end(&local_stats->lock);
@@ -143,6 +145,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue
queue)
write_seqcount_begin(&global_stats->lock);
global_stats->start_ns = now;
write_seqcount_end(&global_stats->lock);
+
+ preempt_enable();
}
static void
@@ -164,8 +168,10 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue
queue)
struct v3d_stats *local_stats = &file->stats[queue];
u64 now = local_clock();
+ preempt_disable();
v3d_stats_update(local_stats, now);
v3d_stats_update(global_stats, now);
+ preempt_enable();
Although `v3d_stats_update()` is only used here, shouldn't we move
`preempt_disable()` and `preempt_enable()` inside of the
`v3d_stats_update()` function? This way, we can guarantee that any
future uses will disable preemption.
With that, you can add my:
Acked-by: Maíra Canal
Best Regards,
- Maíra
}
static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
