[Intel-gfx] [RFC v2 2/3] drm/i915/pmu: serve global events and support perf stat
This patch should probably be squashed with Tvrtko's PMU enabling
patch...
Making perf-stat workable with i915 PMU. The major point is that
current implementation of i915 PMU exposes global counter rather
thatn per-task counters. Thus, required changes are:
* Register PMU with .task_ctx_nr=perf_invalid_context
* Expose cpumask for the PMU with the single CPU in the mask
* Properly support pmu->stop(): it should call pmu->read()
* Properly support pmu->del(): it should call stop(event, PERF_EF_UPDATE)
Examples:
perf stat -e i915/rcs0-busy/ workload.sh
This should error out with "failed to read counter" because the
requested counter can't be queried in per-task mode (which is the case).
perf stat -e i915/rcs0-busy/ -a workload.sh
perf stat -e i915/rcs0-busy/ -a -C0 workload.sh
These 2 commands should give the same result with the correct counter
values. Counter value will be queried once in the end of the wrokload.
The example output would be:
Performance counter stats for 'system wide':
649,547,987 i915/rcs0-busy/
1.895530680 seconds time elapsed
perf stat -e i915/rcs0-busy/ -a -I 100 workload.sh
This will query counter perdiodically (each 100ms) and dump output:
0.100108369 4,137,438 i915/rcs0-busy/
i915/rcs0-busy/: 37037414 100149071 100149071
0.200249024 37,037,414 i915/rcs0-busy/
i915/rcs0-busy/: 36935429 100145077 100145077
0.300391916 36,935,429 i915/rcs0-busy/
i915/rcs0-busy/: 34262017 100126136 100126136
0.400518037 34,262,017 i915/rcs0-busy/
i915/rcs0-busy/: 34539960 100126217 100126217
v1: Make pmu.busy_stats a refcounter to avoid busy stats going away
with some deleted event.
v2: Expose cpumask for i915 PMU to avoid multiple events creation of
the same type followed by counter aggregation by perf-stat.
Change-Id: I7d1abe747a4399196e72253f7b66441a6528dbee
Signed-off-by: Dmitry Rogozhkin
Cc: Tvrtko Ursulin
Cc: Peter Zijlstra
---
drivers/gpu/drm/i915/i915_pmu.c | 71 +
drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
2 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index bcdf2bc..c551d64 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -15,6 +15,8 @@
#define ENGINE_SAMPLE_BITS (16)
+static cpumask_t i915_pmu_cpumask = CPU_MASK_CPU0;
+
static u8 engine_config_sample(u64 config)
{
return config & I915_PMU_SAMPLE_MASK;
@@ -285,16 +287,24 @@ static int i915_pmu_event_init(struct perf_event *event)
{
struct drm_i915_private *i915 =
container_of(event->pmu, typeof(*i915), pmu.base);
+ int cpu;
int ret;
- /* XXX ideally only want pid == -1 && cpu == -1 */
-
if (event->attr.type != event->pmu->type)
return -ENOENT;
if (has_branch_stack(event))
return -EOPNOTSUPP;
+ if (event->cpu < 0)
+ return -EINVAL;
+
+ cpu = cpumask_any_and(&i915_pmu_cpumask,
+ topology_sibling_cpumask(event->cpu));
+
+ if (cpu >= nr_cpu_ids)
+ return -ENODEV;
+
ret = 0;
if (is_engine_event(event)) {
ret = engine_event_init(event);
@@ -314,6 +324,7 @@ static int i915_pmu_event_init(struct perf_event *event)
if (ret)
return ret;
+ event->cpu = cpu;
if (!event->parent)
event->destroy = i915_pmu_event_destroy;
@@ -469,11 +480,16 @@ static void i915_pmu_enable(struct perf_event *event)
ns_to_ktime(PERIOD), 0,
HRTIMER_MODE_REL_PINNED);
i915->pmu.timer_enabled = true;
- } else if (is_engine_event(event) && engine_needs_busy_stats(engine) &&
- !engine->pmu.busy_stats) {
- engine->pmu.busy_stats = true;
- if (!cancel_delayed_work(&engine->pmu.disable_busy_stats))
- queue_work(i915->wq, &engine->pmu.enable_busy_stats);
+ } else if (is_engine_event(event) && engine_needs_busy_stats(engine)) {
+ /* Enable busy stats for the first event demanding it, next
+* events just reference counter. So, if some events will go
+* away, we will still have busy stats enabled till remaining
+* events still use them.
+*/
+ if (engine->pmu.busy_stats++ == 0) {
+ if
(!cancel_delayed_work(&engine->pmu.disable_busy_stats))
+ queue_work(i915->wq,
&engine->pmu.enable_busy_stats);
+ }
}
spin_unlock_irqrestore(&i915->pmu.lock, flags);
@@ -495,16 +511,16 @@ static void i915_pmu_disable(struct perf_event *event)
enum intel_engine_id id;
engine = intel_engine_l
Re: [Intel-gfx] [RFC v2 2/3] drm/i915/pmu: serve global events and support perf stat
On Wed, 2017-08-23 at 08:26 -0700, Dmitry Rogozhkin wrote: > +static cpumask_t i915_pmu_cpumask = CPU_MASK_CPU0; Peter, this hardcoding of cpumask to use CPU0 works, but should I implement something smarter or this will be sufficient? I see that cstate.c you have pointed me to tries to track CPUs going online/offline and migrates PMU context to another CPU if selected one went offline. Should I follow this way? If I should track CPUs going online/offline, then I have questions: 1. How I should register tracking callbacks? I see that cstate.c registers CPUHP_AP_PERF_X86_CSTATE_STARTING and CPUHP_AP_PERF_X86_CSTATE_ONLINE, uncore.c registers CPUHP_AP_PERF_X86_UNCORE_ONLINE. What I should use? I incline to UNCORE. 2. If I will register for, say UNCORE, then how double registrations will be handled if both uncore.c and i915.c will register callbacks? Any conflict here? 3. What I should pass as 2nd argument? Will "perf/x86/intel/i915:online" be ok? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v2 2/3] drm/i915/pmu: serve global events and support perf stat
Peter, any comments? On Wed, 2017-08-23 at 23:38 +, Rogozhkin, Dmitry V wrote: > On Wed, 2017-08-23 at 08:26 -0700, Dmitry Rogozhkin wrote: > > +static cpumask_t i915_pmu_cpumask = CPU_MASK_CPU0; > > > Peter, this hardcoding of cpumask to use CPU0 works, but should I > implement something smarter or this will be sufficient? I see that > cstate.c you have pointed me to tries to track CPUs going online/offline > and migrates PMU context to another CPU if selected one went offline. > Should I follow this way? > > If I should track CPUs going online/offline, then I have questions: > 1. How I should register tracking callbacks? I see that cstate.c > registers CPUHP_AP_PERF_X86_CSTATE_STARTING and > CPUHP_AP_PERF_X86_CSTATE_ONLINE, uncore.c registers > CPUHP_AP_PERF_X86_UNCORE_ONLINE. What I should use? I incline to UNCORE. > 2. If I will register for, say UNCORE, then how double registrations > will be handled if both uncore.c and i915.c will register callbacks? Any > conflict here? > 3. What I should pass as 2nd argument? Will "perf/x86/intel/i915:online" > be ok? > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v2 2/3] drm/i915/pmu: serve global events and support perf stat
On Wed, Aug 23, 2017 at 11:38:43PM +, Rogozhkin, Dmitry V wrote: > On Wed, 2017-08-23 at 08:26 -0700, Dmitry Rogozhkin wrote: > > +static cpumask_t i915_pmu_cpumask = CPU_MASK_CPU0; > > Peter, this hardcoding of cpumask to use CPU0 works, but should I > implement something smarter or this will be sufficient? I see that > cstate.c you have pointed me to tries to track CPUs going online/offline > and migrates PMU context to another CPU if selected one went offline. > Should I follow this way? Yes.. x86 used to not allow hotplug of CPU0, but they 'fixed' that :/ And the perf core needs _a_ valid CPU to run things from, which leaves you having to track online/offline things. Now, I suppose its all fairly similar for a lot of uncore PMUs, so maybe you can pull some of this into a library and avoid the endless duplication between all (most?) uncore driveres. > If I should track CPUs going online/offline, then I have questions: > 1. How I should register tracking callbacks? I see that cstate.c > registers CPUHP_AP_PERF_X86_CSTATE_STARTING and > CPUHP_AP_PERF_X86_CSTATE_ONLINE, uncore.c registers > CPUHP_AP_PERF_X86_UNCORE_ONLINE. What I should use? I incline to UNCORE. Egads, what a mess :/ Clearly I didn't pay too much attention there. So ideally we'd not hate a state per PMU, and __cpuhp_setup_state_cpuslocked() has a .multi_instance argument that allows reuse of a state. So yes, please use the PERF_X86_UNCORE ones if possible. > 2. If I will register for, say UNCORE, then how double registrations > will be handled if both uncore.c and i915.c will register callbacks? Any > conflict here? Should work with .multi_instance I _think_, I've not had the pleasure of using the new and improved CPU hotplug infrastructure much. > 3. What I should pass as 2nd argument? Will "perf/x86/intel/i915:online" > be ok? Yeah, whatever I think.. something unique. Someone or something will eventually yell if its no good I suppose ;-) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v2 2/3] drm/i915/pmu: serve global events and support perf stat
On Tue, 2017-08-29 at 11:28 +0200, Peter Zijlstra wrote: > On Wed, Aug 23, 2017 at 11:38:43PM +, Rogozhkin, Dmitry V wrote: > > On Wed, 2017-08-23 at 08:26 -0700, Dmitry Rogozhkin wrote: > > > +static cpumask_t i915_pmu_cpumask = CPU_MASK_CPU0; > > > > Peter, this hardcoding of cpumask to use CPU0 works, but should I > > implement something smarter or this will be sufficient? I see that > > cstate.c you have pointed me to tries to track CPUs going online/offline > > and migrates PMU context to another CPU if selected one went offline. > > Should I follow this way? > > Yes.. x86 used to not allow hotplug of CPU0, but they 'fixed' that :/ > > And the perf core needs _a_ valid CPU to run things from, which leaves > you having to track online/offline things. > > Now, I suppose its all fairly similar for a lot of uncore PMUs, so maybe > you can pull some of this into a library and avoid the endless > duplication between all (most?) uncore driveres. > > > If I should track CPUs going online/offline, then I have questions: > > 1. How I should register tracking callbacks? I see that cstate.c > > registers CPUHP_AP_PERF_X86_CSTATE_STARTING and > > CPUHP_AP_PERF_X86_CSTATE_ONLINE, uncore.c registers > > CPUHP_AP_PERF_X86_UNCORE_ONLINE. What I should use? I incline to UNCORE. > > Egads, what a mess :/ Clearly I didn't pay too much attention there. > > So ideally we'd not hate a state per PMU, and > __cpuhp_setup_state_cpuslocked() has a .multi_instance argument that > allows reuse of a state. > > So yes, please use the PERF_X86_UNCORE ones if possible. > > > 2. If I will register for, say UNCORE, then how double registrations > > will be handled if both uncore.c and i915.c will register callbacks? Any > > conflict here? > > Should work with .multi_instance I _think_, I've not had the pleasure of > using the new and improved CPU hotplug infrastructure much. > > > 3. What I should pass as 2nd argument? Will "perf/x86/intel/i915:online" > > be ok? > > Yeah, whatever I think.. something unique. Someone or something will > eventually yell if its no good I suppose ;-) I figured out how to track cpus online/offline status in PMU. Here is a question however. What is the reason for uncore PMUs (cstate.c for example) to register for cpus other than cpu0? I see it registers for first thread of each cpu, on my 8 logical-core systems it registers for cpu0-3 it seems. If they register for few cpus then perf-stat will aggregate counters which can be disabled with '-A, --no-aggr' option. Ok... but they could register for just cpu0. Besides, it looks like on Linux cpu0 can't go offline at all at least of x86 architecture. Peter, could you, please, clarify the reasoning to register designated readers of uncore PMU for few CPUs? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v2 2/3] drm/i915/pmu: serve global events and support perf stat
On Wed, Aug 30, 2017 at 05:24:30PM +, Rogozhkin, Dmitry V wrote: > I figured out how to track cpus online/offline status in PMU. Here is a > question however. What is the reason for uncore PMUs (cstate.c for > example) to register for cpus other than cpu0? I see it registers for > first thread of each cpu, on my 8 logical-core systems it registers for > cpu0-3 it seems. The other answer is that on multi-socket, C-state needs more than 1 CPU. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v2 2/3] drm/i915/pmu: serve global events and support perf stat
On Wed, Aug 30, 2017 at 05:24:30PM +, Rogozhkin, Dmitry V wrote: > Ok... but they could register for just cpu0. Besides, it looks like on > Linux cpu0 can't go offline at all at least of x86 architecture. Peter, > could you, please, clarify the reasoning to register designated readers > of uncore PMU for few CPUs? arch/x86/Kconfig:config BOOTPARAM_HOTPLUG_CPU0 means we cannot rely on CPU0 being present, even on x86. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
