| Message ID | 20240812091218.70317-1-tursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/v3d: Disable preemption while updating GPU stats | expand |
Hi Tvrtko, On 8/12/24 06:12, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > 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 <tvrtko.ursulin@igalia.com> > Fixes: 6abe93b621ab ("drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt handler") > Cc: Maíra Canal <mcanal@igalia.com> > Cc: <stable@vger.kernel.org> # 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 <mcanal@igalia.com> Best Regards, - Maíra > } > > static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
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)