Skip to content

Commit

Permalink
drm/i915/pmu: Fix PMU enable vs execlists tasklet race
Browse files Browse the repository at this point in the history 
Commit 99e48bf ("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: 99e48bf ("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
  • Loading branch information
Tvrtko Ursulin committed Feb 6, 2018
1 parent 4b6ce68 commit b2f78cd
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 78 deletions.
98 changes: 34 additions & 64 deletions drivers/gpu/drm/i915/i915_pmu.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -457,12 +487,6 @@ static void i915_pmu_event_read(struct perf_event *event)
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 =
Expand Down Expand Up @@ -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]++;
}

/*
Expand All @@ -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 =
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -1009,21 +987,13 @@ void i915_pmu_register(struct drm_i915_private *i915)

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;

WARN_ON(i915->pmu.enable);

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);
Expand Down
14 changes: 0 additions & 14 deletions drivers/gpu/drm/i915/intel_ringbuffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/*
Expand Down

0 comments on commit b2f78cd

Please sign in to comment.