drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt handler

In V3D, the conclusion of a job is indicated by a IRQ. When a job
finishes, then we update the local and the global GPU stats of that
queue. But, while the GPU stats are being updated, a user might be
reading the stats from sysfs or fdinfo.

For example, on `gpu_stats_show()`, we could think about a scenario where
`v3d->queue[queue].start_ns != 0`, then an interrupt happens, we update
the value of `v3d->queue[queue].start_ns` to 0, we come back to
`gpu_stats_show()` to calculate `active_runtime` and now,
`active_runtime = timestamp`.

In this simple example, the user would see a spike in the queue usage,
that didn't match reality.

In order to address this issue properly, use a seqcount to protect read
and write sections of the code.

Fixes: 09a93cc4f7d1 ("drm/v3d: Implement show_fdinfo() callback for GPU usage stats")
Reported-by: Tvrtko Ursulin <tursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240420213632.339941-7-mcanal@igalia.com

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 2ec359ed2def020d8cf39beb2ba6a55a365b597f..28b7ddce774770a1af5f28ef22ad8386603a2e67 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -121,6 +121,7 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
                                      1, NULL);
 
                memset(&v3d_priv->stats[i], 0, sizeof(v3d_priv->stats[i]));
+               seqcount_init(&v3d_priv->stats[i].lock);
        }
 
        v3d_perfmon_open_file(v3d_priv);
@@ -145,10 +146,15 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file)
 void v3d_get_stats(const struct v3d_stats *stats, u64 timestamp,
                   u64 *active_runtime, u64 *jobs_completed)
 {
-       *active_runtime = stats->enabled_ns;
-       if (stats->start_ns)
-               *active_runtime += timestamp - stats->start_ns;
-       *jobs_completed = stats->jobs_completed;
+       unsigned int seq;
+
+       do {
+               seq = read_seqcount_begin(&stats->lock);
+               *active_runtime = stats->enabled_ns;
+               if (stats->start_ns)
+                       *active_runtime += timestamp - stats->start_ns;
+               *jobs_completed = stats->jobs_completed;
+       } while (read_seqcount_retry(&stats->lock, seq));
 }
 
 static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index ff06dc1cc078a419d9766842e8cb77ac805ab25e..a2c516fe6d79699853c501700ab7b354ae3cbc35 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -40,6 +40,13 @@ struct v3d_stats {
        u64 start_ns;
        u64 enabled_ns;
        u64 jobs_completed;
+
+       /*
+        * This seqcount is used to protect the access to the GPU stats
+        * variables. It must be used as, while we are reading the stats,
+        * IRQs can happen and the stats can be updated.
+        */
+       seqcount_t lock;
 };
 
 struct v3d_queue_state {

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 0086081a926185f945ce9c78e791d6a44a2377a6..da8faf3b90116588330f27d135f37ef107de9bab 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -251,6 +251,7 @@ v3d_gem_init(struct drm_device *dev)
 
                queue->fence_context = dma_fence_context_alloc(1);
                memset(&queue->stats, 0, sizeof(queue->stats));
+               seqcount_init(&queue->stats.lock);
        }
 
        spin_lock_init(&v3d->mm_lock);

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index b9614944931c81c051fa92839d9908461ea3295f..7cd8c335cd9b7e191513f8ad8ee8f9e7d4673b24 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -114,16 +114,23 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
        struct v3d_stats *local_stats = &file->stats[queue];
        u64 now = local_clock();
 
+       write_seqcount_begin(&local_stats->lock);
        local_stats->start_ns = now;
+       write_seqcount_end(&local_stats->lock);
+
+       write_seqcount_begin(&global_stats->lock);
        global_stats->start_ns = now;
+       write_seqcount_end(&global_stats->lock);
 }
 
 static void
 v3d_stats_update(struct v3d_stats *stats, u64 now)
 {
+       write_seqcount_begin(&stats->lock);
        stats->enabled_ns += now - stats->start_ns;
        stats->jobs_completed++;
        stats->start_ns = 0;
+       write_seqcount_end(&stats->lock);
 }
 
 void