| Message ID | 20240813102505.80512-2-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/13/24 07:25, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > Lockdep thinks our seqcount_t usage is unsafe because the update path can > be both from irq and worker context: > > [ ] ================================ > [ ] WARNING: inconsistent lock state > [ ] 6.10.3-v8-16k-numa #159 Tainted: G WC > [ ] -------------------------------- > [ ] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage. > [ ] swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes: > [ ] ffff80003d7c08d0 (&v3d_priv->stats[i].lock){?.+.}-{0:0}, at: v3d_irq+0xc8/0x660 [v3d] > [ ] {HARDIRQ-ON-W} state was registered at: > [ ] lock_acquire+0x1f8/0x328 > [ ] v3d_job_start_stats.isra.0+0xd8/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 > [ ] irq event stamp: 337094 > [ ] hardirqs last enabled at (337093): [<ffffc0008144ce7c>] default_idle_call+0x11c/0x140 > [ ] hardirqs last disabled at (337094): [<ffffc0008144a354>] el1_interrupt+0x24/0x58 > [ ] softirqs last enabled at (337082): [<ffffc00080061d90>] handle_softirqs+0x4e0/0x538 > [ ] softirqs last disabled at (337073): [<ffffc00080010364>] __do_softirq+0x1c/0x28 > [ ] > other info that might help us debug this: > [ ] Possible unsafe locking scenario: > > [ ] CPU0 > [ ] ---- > [ ] lock(&v3d_priv->stats[i].lock); > [ ] <Interrupt> > [ ] lock(&v3d_priv->stats[i].lock); > [ ] > *** DEADLOCK *** > > [ ] no locks held by swapper/0/0. > [ ] > stack backtrace: > [ ] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G WC 6.10.3-v8-16k-numa #159 > [ ] Hardware name: Raspberry Pi 5 Model B Rev 1.0 (DT) > [ ] Call trace: > [ ] dump_backtrace+0x170/0x1b8 > [ ] show_stack+0x20/0x38 > [ ] dump_stack_lvl+0xb4/0xd0 > [ ] dump_stack+0x18/0x28 > [ ] print_usage_bug+0x3cc/0x3f0 > [ ] mark_lock+0x4d0/0x968 > [ ] __lock_acquire+0x784/0x18c8 > [ ] lock_acquire+0x1f8/0x328 > [ ] v3d_job_update_stats+0xec/0x2e0 [v3d] > [ ] v3d_irq+0xc8/0x660 [v3d] > [ ] __handle_irq_event_percpu+0x1f8/0x488 > [ ] handle_irq_event+0x88/0x128 > [ ] handle_fasteoi_irq+0x298/0x408 > [ ] generic_handle_domain_irq+0x50/0x78 > > But it is a false positive because all the queue-stats pairs have their > own lock and jobs are also one at a time. > > Nevertheless we can appease lockdep by disabling local interrupts to make > it see lock usage is consistent. > > 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> Applied to misc/kernel.git (drm-misc-next). Best Regards, - Maíra > --- > drivers/gpu/drm/v3d/v3d_sched.c | 44 ++++++++++++++++++++++++++++++--- > 1 file changed, 40 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c > index cc2e5a89467b..e4fc3d401edd 100644 > --- a/drivers/gpu/drm/v3d/v3d_sched.c > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > @@ -135,8 +135,31 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue) > struct v3d_stats *global_stats = &v3d->queue[queue].stats; > struct v3d_stats *local_stats = &file->stats[queue]; > u64 now = local_clock(); > + unsigned long flags; > > - preempt_disable(); > + /* > + * We only need to disable local interrupts to appease lockdep who > + * otherwise would think v3d_job_start_stats vs v3d_stats_update has an > + * unsafe in-irq vs no-irq-off usage problem. This is a false positive > + * becuase all the locks are per queue and stats type, and all jobs are > + * completely one at a time serialised. More specifically: > + * > + * 1. Locks for GPU queues are updated from interrupt handlers under a > + * spin lock and started here with preemption disabled. > + * > + * 2. Locks for CPU queues are updated from the worker with preemption > + * disabled and equally started here with preemption disabled. > + * > + * Therefore both are consistent. > + * > + * 3. Because next job can only be queued after the previous one has > + * been signaled, and locks are per queue, there is also no scope for > + * the start part to race with the update part. > + */ > + if (IS_ENABLED(CONFIG_LOCKDEP)) > + local_irq_save(flags); > + else > + preempt_disable(); > > write_seqcount_begin(&local_stats->lock); > local_stats->start_ns = now; > @@ -146,7 +169,10 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue) > global_stats->start_ns = now; > write_seqcount_end(&global_stats->lock); > > - preempt_enable(); > + if (IS_ENABLED(CONFIG_LOCKDEP)) > + local_irq_restore(flags); > + else > + preempt_enable(); > } > > static void > @@ -167,11 +193,21 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) > struct v3d_stats *global_stats = &v3d->queue[queue].stats; > struct v3d_stats *local_stats = &file->stats[queue]; > u64 now = local_clock(); > + unsigned long flags; > + > + /* See comment in v3d_job_start_stats() */ > + if (IS_ENABLED(CONFIG_LOCKDEP)) > + local_irq_save(flags); > + else > + preempt_disable(); > > - preempt_disable(); > v3d_stats_update(local_stats, now); > v3d_stats_update(global_stats, now); > - preempt_enable(); > + > + if (IS_ENABLED(CONFIG_LOCKDEP)) > + local_irq_restore(flags); > + else > + preempt_enable(); > } > > 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 cc2e5a89467b..e4fc3d401edd 100644 --- a/drivers/gpu/drm/v3d/v3d_sched.c +++ b/drivers/gpu/drm/v3d/v3d_sched.c @@ -135,8 +135,31 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue) struct v3d_stats *global_stats = &v3d->queue[queue].stats; struct v3d_stats *local_stats = &file->stats[queue]; u64 now = local_clock(); + unsigned long flags; - preempt_disable(); + /* + * We only need to disable local interrupts to appease lockdep who + * otherwise would think v3d_job_start_stats vs v3d_stats_update has an + * unsafe in-irq vs no-irq-off usage problem. This is a false positive + * becuase all the locks are per queue and stats type, and all jobs are + * completely one at a time serialised. More specifically: + * + * 1. Locks for GPU queues are updated from interrupt handlers under a + * spin lock and started here with preemption disabled. + * + * 2. Locks for CPU queues are updated from the worker with preemption + * disabled and equally started here with preemption disabled. + * + * Therefore both are consistent. + * + * 3. Because next job can only be queued after the previous one has + * been signaled, and locks are per queue, there is also no scope for + * the start part to race with the update part. + */ + if (IS_ENABLED(CONFIG_LOCKDEP)) + local_irq_save(flags); + else + preempt_disable(); write_seqcount_begin(&local_stats->lock); local_stats->start_ns = now; @@ -146,7 +169,10 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue) global_stats->start_ns = now; write_seqcount_end(&global_stats->lock); - preempt_enable(); + if (IS_ENABLED(CONFIG_LOCKDEP)) + local_irq_restore(flags); + else + preempt_enable(); } static void @@ -167,11 +193,21 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue) struct v3d_stats *global_stats = &v3d->queue[queue].stats; struct v3d_stats *local_stats = &file->stats[queue]; u64 now = local_clock(); + unsigned long flags; + + /* See comment in v3d_job_start_stats() */ + if (IS_ENABLED(CONFIG_LOCKDEP)) + local_irq_save(flags); + else + preempt_disable(); - preempt_disable(); v3d_stats_update(local_stats, now); v3d_stats_update(global_stats, now); - preempt_enable(); + + if (IS_ENABLED(CONFIG_LOCKDEP)) + local_irq_restore(flags); + else + preempt_enable(); } static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)