UbiquitousOS
/
OpenCloudOS-Kernel
mirror of https://gitee.com/OpenCloudOS/OpenCloudOS-Kernel.git
11
0
Fork 0
Code Issues Projects Releases Wiki Activity

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

Commit 99e48bf98d ("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: 99e48bf98d ("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
This commit is contained in:
Tvrtko Ursulin 2018-02-05 09:34:48 +00:00
parent 4b6ce6810a
commit b2f78cda26
2 changed files with 34 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

98
drivers/gpu/drm/i915/i915_pmu.c
View File

@ -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);

14
drivers/gpu/drm/i915/intel_ringbuffer.h
View File

@ -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;
/*