drm/i915/pmu: Fix PMU enable vs execlists tasklet race

Commit 99e48bf98dd0 ("drm/i915: Lock out execlist tasklet while peeking
inside for busy-stats") added a tasklet_disable call in busy stats
enabling, but we failed to understand that the PMU enable callback runs
as an hard IRQ (IPI).

Consequence of this is that the PMU enable callback can interrupt the
execlists tasklet, and will then deadlock when it calls
intel_engine_stats_enable->tasklet_disable.

To fix this, I realized it is possible to move the engine stats enablement
and disablement to PMU event init and destroy hooks. This allows for much
simpler implementation since those hooks run in normal context (can
sleep).

v2: Extract engine_event_destroy. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 99e48bf98dd0 ("drm/i915: Lock out execlist tasklet while peeking inside for busy-stats")
Testcase: igt/perf_pmu/enable-race-*
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20180205093448.13877-1-tvrtko.ursulin@linux.intel.com

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index ecb0198bfb7ab715a8c61411dce968ed9bf6ba01..1c440460255ddcd0ff3916107b705fc881d254d6 100644 (file)
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -285,9 +285,29 @@ static u64 count_interrupts(struct drm_i915_private *i915)
        return sum;
 }
 
+static void engine_event_destroy(struct perf_event *event)
+{
+       struct drm_i915_private *i915 =
+               container_of(event->pmu, typeof(*i915), pmu.base);
+       struct intel_engine_cs *engine;
+
+       engine = intel_engine_lookup_user(i915,
+                                         engine_event_class(event),
+                                         engine_event_instance(event));
+       if (WARN_ON_ONCE(!engine))
+               return;
+
+       if (engine_event_sample(event) == I915_SAMPLE_BUSY &&
+           intel_engine_supports_stats(engine))
+               intel_disable_engine_stats(engine);
+}
+
 static void i915_pmu_event_destroy(struct perf_event *event)
 {
        WARN_ON(event->parent);
+
+       if (is_engine_event(event))
+               engine_event_destroy(event);
 }
 
 static int
@@ -340,13 +360,23 @@ static int engine_event_init(struct perf_event *event)
        struct drm_i915_private *i915 =
                container_of(event->pmu, typeof(*i915), pmu.base);
        struct intel_engine_cs *engine;
+       u8 sample;
+       int ret;
 
        engine = intel_engine_lookup_user(i915, engine_event_class(event),
                                          engine_event_instance(event));
        if (!engine)
                return -ENODEV;
 
-       return engine_event_status(engine, engine_event_sample(event));
+       sample = engine_event_sample(event);
+       ret = engine_event_status(engine, sample);
+       if (ret)
+               return ret;
+
+       if (sample == I915_SAMPLE_BUSY && intel_engine_supports_stats(engine))
+               ret = intel_enable_engine_stats(engine);
+
+       return ret;
 }
 
 static int i915_pmu_event_init(struct perf_event *event)
@@ -402,7 +432,7 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
                if (WARN_ON_ONCE(!engine)) {
                        /* Do nothing */
                } else if (sample == I915_SAMPLE_BUSY &&
-                          engine->pmu.busy_stats) {
+                          intel_engine_supports_stats(engine)) {
                        val = ktime_to_ns(intel_engine_get_busy_time(engine));
                } else {
                        val = engine->pmu.sample[sample].cur;
@@ -457,12 +487,6 @@ again:
        local64_add(new - prev, &event->count);
 }
 
-static bool engine_needs_busy_stats(struct intel_engine_cs *engine)
-{
-       return intel_engine_supports_stats(engine) &&
-              (engine->pmu.enable & BIT(I915_SAMPLE_BUSY));
-}
-
 static void i915_pmu_enable(struct perf_event *event)
 {
        struct drm_i915_private *i915 =
@@ -502,21 +526,7 @@ static void i915_pmu_enable(struct perf_event *event)
 
                GEM_BUG_ON(sample >= I915_PMU_SAMPLE_BITS);
                GEM_BUG_ON(engine->pmu.enable_count[sample] == ~0);
-               if (engine->pmu.enable_count[sample]++ == 0) {
-                       /*
-                        * Enable engine busy stats tracking if needed or
-                        * alternatively cancel the scheduled disable.
-                        *
-                        * If the delayed disable was pending, cancel it and
-                        * in this case do not enable since it already is.
-                        */
-                       if (engine_needs_busy_stats(engine) &&
-                           !engine->pmu.busy_stats) {
-                               engine->pmu.busy_stats = true;
-                               if (!cancel_delayed_work(&engine->pmu.disable_busy_stats))
-                                       intel_enable_engine_stats(engine);
-                       }
-               }
+               engine->pmu.enable_count[sample]++;
        }
 
        /*
@@ -529,14 +539,6 @@ static void i915_pmu_enable(struct perf_event *event)
        spin_unlock_irqrestore(&i915->pmu.lock, flags);
 }
 
-static void __disable_busy_stats(struct work_struct *work)
-{
-       struct intel_engine_cs *engine =
-              container_of(work, typeof(*engine), pmu.disable_busy_stats.work);
-
-       intel_disable_engine_stats(engine);
-}
-
 static void i915_pmu_disable(struct perf_event *event)
 {
        struct drm_i915_private *i915 =
@@ -560,26 +562,8 @@ static void i915_pmu_disable(struct perf_event *event)
                 * Decrement the reference count and clear the enabled
                 * bitmask when the last listener on an event goes away.
                 */
-               if (--engine->pmu.enable_count[sample] == 0) {
+               if (--engine->pmu.enable_count[sample] == 0)
                        engine->pmu.enable &= ~BIT(sample);
-                       if (!engine_needs_busy_stats(engine) &&
-                           engine->pmu.busy_stats) {
-                               engine->pmu.busy_stats = false;
-                               /*
-                                * We request a delayed disable to handle the
-                                * rapid on/off cycles on events, which can
-                                * happen when tools like perf stat start, in a
-                                * nicer way.
-                                *
-                                * In addition, this also helps with busy stats
-                                * accuracy with background CPU offline/online
-                                * migration events.
-                                */
-                               queue_delayed_work(system_wq,
-                                                  &engine->pmu.disable_busy_stats,
-                                                  round_jiffies_up_relative(HZ));
-                       }
-               }
        }
 
        GEM_BUG_ON(bit >= I915_PMU_MASK_BITS);
@@ -956,8 +940,6 @@ static void i915_pmu_unregister_cpuhp_state(struct drm_i915_private *i915)
 
 void i915_pmu_register(struct drm_i915_private *i915)
 {
-       struct intel_engine_cs *engine;
-       enum intel_engine_id id;
        int ret;
 
        if (INTEL_GEN(i915) <= 2) {
@@ -985,10 +967,6 @@ void i915_pmu_register(struct drm_i915_private *i915)
        hrtimer_init(&i915->pmu.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
        i915->pmu.timer.function = i915_sample;
 
-       for_each_engine(engine, i915, id)
-               INIT_DELAYED_WORK(&engine->pmu.disable_busy_stats,
-                                 __disable_busy_stats);
-
        ret = perf_pmu_register(&i915->pmu.base, "i915", -1);
        if (ret)
                goto err;
@@ -1009,9 +987,6 @@ err:
 
 void i915_pmu_unregister(struct drm_i915_private *i915)
 {
-       struct intel_engine_cs *engine;
-       enum intel_engine_id id;
-
        if (!i915->pmu.base.event_init)
                return;
 
@@ -1019,11 +994,6 @@ void i915_pmu_unregister(struct drm_i915_private *i915)
 
        hrtimer_cancel(&i915->pmu.timer);
 
-       for_each_engine(engine, i915, id) {
-               GEM_BUG_ON(engine->pmu.busy_stats);
-               flush_delayed_work(&engine->pmu.disable_busy_stats);
-       }
-
        i915_pmu_unregister_cpuhp_state(i915);
 
        perf_pmu_unregister(&i915->pmu.base);

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c5ff203e42d6a8627d3c6bb18c589a103311d0c3..a0e7a6c2a57cd8f0b66f27e7e5dcc874857fd649 100644 (file)
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -366,20 +366,6 @@ struct intel_engine_cs {
                 */
 #define I915_ENGINE_SAMPLE_MAX (I915_SAMPLE_SEMA + 1)
                struct i915_pmu_sample sample[I915_ENGINE_SAMPLE_MAX];
-               /**
-                * @busy_stats: Has enablement of engine stats tracking been
-                *              requested.
-                */
-               bool busy_stats;
-               /**
-                * @disable_busy_stats: Work item for busy stats disabling.
-                *
-                * Same as with @enable_busy_stats action, with the difference
-                * that we delay it in case there are rapid enable-disable
-                * actions, which can happen during tool startup (like perf
-                * stat).
-                */
-               struct delayed_work disable_busy_stats;
        } pmu;
 
        /*