perf_events: improve x86 event scheduling
Subsystems (2)
PATCH 1/1 perf_events: improve x86 event scheduling
This patch improves event scheduling by maximizing the use of PMU registers regardless of the order in which events are created in a group.
The algorithm takes into account the list of counter constraints for each event. It assigns events to counters from the most constrained, i.e., works on only one counter, to the least constrained, i.e., works on any counter.
Intel Fixed counter events and the BTS special event are also handled via this algorithm which is designed to be fairly generic.
The patch also updates the validation of an event to use the scheduling algorithm. This will cause early failure in perf_event_open().
This is the 2nd version of this patch. It follows the model used by PPC, by running the scheduling algorithm and the actual assignment separately. Actual assignment takes place in hw_perf_enable() whereas scheduling is implemented in hw_perf_group_sched_in() and x86_pmu_enable().
--
include/asm/perf_event.h | 17 + kernel/cpu/perf_event.c | 694 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 494 insertions(+), 217 deletions(-)
arch/x86/include/asm/perf_event.h +15 -2
@@ -19,6 +19,7 @@#define MSR_ARCH_PERFMON_EVENTSEL1 0x187#define ARCH_PERFMON_EVENTSEL0_ENABLE (1 << 22) +#define ARCH_PERFMON_EVENTSEL_ANY (1 << 21)#define ARCH_PERFMON_EVENTSEL_INT (1 << 20)#define ARCH_PERFMON_EVENTSEL_OS (1 << 17)#define ARCH_PERFMON_EVENTSEL_USR (1 << 16)
@@ -26,7 +27,14 @@/* * Includes eventsel and unit mask as well: */ -#define ARCH_PERFMON_EVENT_MASK 0xffff + + +#define INTEL_ARCH_EVTSEL_MASK 0x000000FFULL +#define INTEL_ARCH_UNIT_MASK 0x0000FF00ULL +#define INTEL_ARCH_EDGE_MASK 0x00040000ULL +#define INTEL_ARCH_INV_MASK 0x00800000ULL +#define INTEL_ARCH_CNT_MASK 0xFF000000ULL +#define INTEL_ARCH_EVENT_MASK (INTEL_ARCH_UNIT_MASK|INTEL_ARCH_EVTSEL_MASK)/* * filter mask to validate fixed counter events.
@@ -37,7 +45,12 @@* The other filters are supported by fixed counters. * The any-thread option is supported starting with v3. */ -#define ARCH_PERFMON_EVENT_FILTER_MASK 0xff840000 +#define INTEL_ARCH_FIXED_MASK \ + (INTEL_ARCH_CNT_MASK| \ + INTEL_ARCH_INV_MASK| \ + INTEL_ARCH_EDGE_MASK|\ + INTEL_ARCH_UNIT_MASK|\ + INTEL_ARCH_EVENT_MASK)#define ARCH_PERFMON_UNHALTED_CORE_CYCLES_SEL 0x3c#define ARCH_PERFMON_UNHALTED_CORE_CYCLES_UMASK (0x00 << 8)
arch/x86/kernel/cpu/perf_event.c +479 -215
879 lines not on this page · raw message
Peter Zijlstra
maintainerQuoted 16 lines · Stephane Eranian
This patch improves event scheduling by maximizing the use of PMU registers regardless of the order in which events are created in a group.
The algorithm takes into account the list of counter constraints for each event. It assigns events to counters from the most constrained, i.e., works on only one counter, to the least constrained, i.e., works on any counter.
Intel Fixed counter events and the BTS special event are also handled via this algorithm which is designed to be fairly generic.
The patch also updates the validation of an event to use the scheduling algorithm. This will cause early failure in
perf_event_open().
Quoted 5 lines
This is the 2nd version of this patch. It follows the model used by PPC, by running the scheduling algorithm and the actual assignment separately. Actual assignment takes place in
hw_perf_enable()whereas scheduling is implemented inhw_perf_group_sched_in()andx86_pmu_enable().
Looks very good, will have to reread it again in the morning to look for more details but from an initial reading its fine.
One concern I do have is the loss of error checking on event_sched_in()'s event->pmu->enable(), that could be another 'hardware' PMU like breakpoints, in which case it could fail.
Not sure what to do with that.. maybe we should not allow mixing different hardware PMU events, but only 1 hardware + software events, in which case the below patch should retain some of the is_software_event()s.
Frederic, Paul?
Quoted 25 lines
Signed-off-by Stephane Eranian <[email protected]>--
arch/x86/include/asm/perf_event.h +1
-0@@ -19,6 +19,7 @@#define MSR_ARCH_PERFMON_EVENTSEL1 0x187#define ARCH_PERFMON_EVENTSEL0_ENABLE (1 << 22) +#define ARCH_PERFMON_EVENTSEL_ANY (1 << 21)#define ARCH_PERFMON_EVENTSEL_INT (1 << 20)#define ARCH_PERFMON_EVENTSEL_OS (1 << 17)#define ARCH_PERFMON_EVENTSEL_USR (1 << 16)arch/x86/kernel/cpu/perf_event.c +0
-0@@ -1343,6 +1579,13 @@ intel_pmu_enable_fixed(struct hw_perf_event *hwc, int __idx) bits |=0x2 ;if (hwc->config &ARCH_PERFMON_EVENTSEL_OS ) bits |=0x1 ;
arch/x86/kernel/cpu/perf_event.c intel_pmu_enable_fixed(struct hw_perf_event *hwc, int __idx) · line 1591
Quoted 10 lines
arch/x86/kernel/cpu/perf_event.c +7
-0+ +/* +* ANY bit is supported in v3 and up +*/ +if (x86_pmu.version >2 && hwc->config &ARCH_PERFMON_EVENTSEL_ANY ) + bits |=0x4 ; + bits <<= (idx *4 ); mask =0xfULL << (idx *4 );
This looks like a separate patch all by itself.
Also, the below fixes a few style nits and that is_software_event() usage:
---
arch/x86/kernel/cpu/perf_event.c +15 -6
@@ -225,7 +225,8 @@ static struct event_constraint intel_corEVENT_CONSTRAINT_END }; -static struct event_constraint intel_nehalem_event_constraints[] = { +static struct event_constraint intel_nehalem_event_constraints[] = +{EVENT_CONSTRAINT (0xc0 , (0x3 |(1ULL <<32 )),INTEL_ARCH_FIXED_MASK ),/* INSTRUCTIONS_RETIRED */ EVENT_CONSTRAINT (0x3c , (0x3 |(1ULL <<33 )),INTEL_ARCH_FIXED_MASK ),/* UNHALTED_CORE_CYCLES */ EVENT_CONSTRAINT (0x40 ,0x3 ,INTEL_ARCH_EVENT_MASK ),/* L1D_CACHE_LD */
@@ -241,7 +242,8 @@ static struct event_constraint intel_neh EVENT_CONSTRAINT_END }; -static struct event_constraint intel_gen_event_constraints[] = { +static struct event_constraint intel_gen_event_constraints[] = +{EVENT_CONSTRAINT (0xc0 , (0x3 |(1ULL <<32 )),INTEL_ARCH_FIXED_MASK ),/* INSTRUCTIONS_RETIRED */ EVENT_CONSTRAINT (0x3c , (0x3 |(1ULL <<33 )),INTEL_ARCH_FIXED_MASK ),/* UNHALTED_CORE_CYCLES */ };
@@ -1310,6 +1312,13 @@ skip:return 0 ; } +static const struct pmu pmu; + +static inline bool is_x86_event (struct perf_event *event) +{ +return event->pmu == pmu; +} +/* * dogrp: true if must collect siblings events (group) * returns total number of events and error code
@@ -1324,7 +1333,7 @@ static int collect_events(struct cpu_hw_/* current number of events already accepted */ n = cpuc->n_events; -if (!is_software_event (leader)) { +if (is_x86_event (leader)) {if (n >= max_count)return -ENOSPC ; cpuc->event_list[n] = leader;
@@ -1334,7 +1343,7 @@ static int collect_events(struct cpu_hw_return n;list_for_each_entry (event, &leader->sibling_list, group_entry) { -if (is_software_event (event) || +if (!is_x86_event (event) || event->state ==PERF_EVENT_STATE_OFF )continue ;
@@ -2154,7 +2163,7 @@ static void event_sched_in(struct perf_e event->state =PERF_EVENT_STATE_ACTIVE ; event->oncpu = cpu; event->tstamp_running += event->ctx->time - event->tstamp_stopped; -if (is_software_event (event)) +if (!is_x86_event (event)) event->pmu->enable (event); }
@@ -2209,7 +2218,7 @@ int hw_perf_group_sched_in(struct perf_e* 1 means successful and events are active * This is not quite true because we defer * actual activation until hw_perf_enable() but -* this way we* ensure caller won't try to enable +* this way we ensure caller won't try to enable * individual events */ return 1 ;
Quoted 23 lines · Peter Zijlstra
Quoted 16 lines · Stephane Eranian
This patch improves event scheduling by maximizing the use of PMU registers regardless of the order in which events are created in a group.
The algorithm takes into account the list of counter constraints for each event. It assigns events to counters from the most constrained, i.e., works on only one counter, to the least constrained, i.e., works on any counter.
Intel Fixed counter events and the BTS special event are also handled via this algorithm which is designed to be fairly generic.
The patch also updates the validation of an event to use the scheduling algorithm. This will cause early failure in
perf_event_open().Quoted 5 lines
This is the 2nd version of this patch. It follows the model used by PPC, by running the scheduling algorithm and the actual assignment separately. Actual assignment takes place in
hw_perf_enable()whereas scheduling is implemented inhw_perf_group_sched_in()andx86_pmu_enable().
Quoted 7 lines
Looks very good, will have to reread it again in the morning to look for more details but from an initial reading its fine.
One concern I do have is the loss of error checking on event_sched_in()'s
event->pmu->enable(), that could be another 'hardware' PMU like breakpoints, in which case it could fail.
Well, x86_pmu_enable() does return an error code, so it is up to the upper layer to handle the error gracefully and in particular in perf_ctx_adjust_freq().
Quoted 120 lines
Quoted 25 lines
Signed-off-by Stephane Eranian <[email protected]>--
arch/x86/include/asm/perf_event.h +1
-0@@ -19,6 +19,7 @@ #define MSR_ARCH_PERFMON_EVENTSEL1 0x187 #define ARCH_PERFMON_EVENTSEL0_ENABLE (1 << 22) +#define ARCH_PERFMON_EVENTSEL_ANY (1 << 21) #define ARCH_PERFMON_EVENTSEL_INT (1 << 20) #define ARCH_PERFMON_EVENTSEL_OS (1 << 17) #define ARCH_PERFMON_EVENTSEL_USR (1 << 16)arch/x86/kernel/cpu/perf_event.c +0
-0@@ -1343,6 +1579,13 @@ intel_pmu_enable_fixed(struct hw_perf_event *hwc, int __idx) bits |= 0x2; if (hwc->config & ARCH_PERFMON_EVENTSEL_OS) bits |= 0x1;arch/x86/kernel/cpu/perf_event.c intel_pmu_enable_fixed(struct hw_perf_event *hwc, int __idx) · line 1591
Quoted 10 lines
arch/x86/kernel/cpu/perf_event.c +7
-0+ + /* + * ANY bit is supported in v3 and up + */ + if (x86_pmu.version > 2 && hwc->config & ARCH_PERFMON_EVENTSEL_ANY) + bits |= 0x4; + bits <<= (idx * 4); mask = 0xfULL << (idx * 4);This looks like a separate patch all by itself.
Also, the below fixes a few style nits and that
is_software_event()usage:---
Index:linux-2.6/arch/x86/kernel/cpu/perf_event.c
===================================================================arch/x86/kernel/cpu/perf_event.c +0
-0@@ -225,7 +225,8 @@ static struct event_constraint intel_corEVENT_CONSTRAINT_END
};arch/x86/kernel/cpu/perf_event.c +2
-1-static struct event_constraint intel_nehalem_event_constraints[] = { +static struct event_constraint intel_nehalem_event_constraints[] = +{EVENT_CONSTRAINT(0xc0, (0x3|(1ULL<<32)),
INTEL_ARCH_FIXED_MASK), /*INSTRUCTIONS_RETIRED*/EVENT_CONSTRAINT(0x3c, (0x3|(1ULL<<33)), INTEL_ARCH_FIXED_MASK), /*UNHALTED_CORE_CYCLES*/EVENT_CONSTRAINT(0x40, 0x3, INTEL_ARCH_EVENT_MASK), /*L1D_CACHE_LD*/arch/x86/kernel/cpu/perf_event.c +0
-0@@ -241,7 +242,8 @@ static struct event_constraint intel_nehEVENT_CONSTRAINT_END
};arch/x86/kernel/cpu/perf_event.c +2
-1-static struct event_constraint intel_gen_event_constraints[] = { +static struct event_constraint intel_gen_event_constraints[] = +{EVENT_CONSTRAINT(0xc0, (0x3|(1ULL<<32)),
INTEL_ARCH_FIXED_MASK), /*INSTRUCTIONS_RETIRED*/EVENT_CONSTRAINT(0x3c, (0x3|(1ULL<<33)), INTEL_ARCH_FIXED_MASK), /*UNHALTED_CORE_CYCLES*/ };arch/x86/kernel/cpu/perf_event.c +11
-4@@ -1310,6 +1312,13 @@ skip: return 0; } +static const struct pmu pmu; + +static inline bool is_x86_event (struct perf_event *event) +{ + return event->pmu == pmu; +} + /** dogrp: true if must collect siblings events (group) * returns total number of events and error code @@ -1324,7 +1333,7 @@ static int collect_events(struct cpu_hw_ /* current number of events already accepted */ n = cpuc->n_events; - if (!is_software_event(leader)) { + if (is_x86_event(leader)) { if (n >= max_count) return -ENOSPC; cpuc->event_list[n] = leader;@@ -1334,7 +1343,7 @@ static int collect_events(struct cpu_hw_ return n; list_for_each_entry(event, &leader->sibling_list, group_entry) { - if (is_software_event(event) || + if (!is_x86_event(event) || event->state == PERF_EVENT_STATE_OFF) continue;@@ -2154,7 +2163,7 @@ static void event_sched_in(struct perf_e event->state = PERF_EVENT_STATE_ACTIVE; event->oncpu = cpu; event->tstamp_running += event->ctx->time - event->tstamp_stopped; - if (is_software_event(event)) + if (!is_x86_event(event)) event->pmu->enable(event); }@@ -2209,7 +2218,7 @@ int hw_perf_group_sched_in(struct perf_e* 1 means successful and events are active * This is not quite true because we defer * actual activation until hw_perf_enable() but -* this way we* ensure caller won't try to enable +* this way we ensure caller won't try to enable * individual events */ return 1;
Signature · 8 lines
Stephane Eranian | EMEA Software Engineering Google France | 38 avenue de l'Opéra | 75002 Paris Tel : +33 (0) 1 42 68 53 00 This email may be confidential or privileged. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it went to the wrong person. Thanks
Peter Zijlstra
maintainerQuoted 63 lines · Stephane Eranian
Quoted 4 lines
One concern I do have is the loss of error checking on event_sched_in()'s
event->pmu->enable(), that could be another 'hardware' PMU like breakpoints, in which case it could fail.Well,
x86_pmu_enable()does return an error code, so it is up to the upper layer to handle the error gracefully and in particular inperf_ctx_adjust_freq().(quoted diff) +55
-0+static void event_sched_in(struct perf_event *event, int cpu) +{ + event->state = PERF_EVENT_STATE_ACTIVE; + event->oncpu = cpu; + event->tstamp_running += event->ctx->time - event->tstamp_stopped; + if (is_software_event(event)) + event->pmu->enable(event); +} + +/* + * Called to enable a whole group of events. + * Returns 1 if the group was enabled, or -EAGAIN if it could not be. + * Assumes the caller has disabled interrupts and has + * frozen the PMU with hw_perf_save_disable. + * + * called with PMU disabled. If successful and return value 1, + * then guaranteed to call perf_enable() and hw_perf_enable() + */ +int hw_perf_group_sched_in(struct perf_event *leader, + struct perf_cpu_context *cpuctx, + struct perf_event_context *ctx, int cpu) +{ + struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu); + struct perf_event *sub; + int assign[X86_PMC_IDX_MAX]; + int n, n0, ret; + + n0 = cpuc->n_events; + + n = collect_events(cpuc, leader, true); + if (n < 0) + return n; + + ret = x86_schedule_events(cpuc, n, assign); + if (ret) + return ret; + + /* + * copy new assignment, now we know it is possible + * will be used by hw_perf_enable() + */ + memcpy(cpuc->assign, assign, n*sizeof(int)); + + cpuc->n_events = n; + cpuc->n_added = n - n0; + + n = 1; + event_sched_in(leader, cpu); + list_for_each_entry(sub, &leader->sibling_list, group_entry) { + if (sub->state != PERF_EVENT_STATE_OFF) { + event_sched_in(sub, cpu); + ++n; + } + } + ctx->nr_active += n;
Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* + * 1 means successful and events are active + * This is not quite true because we defer + * actual activation until hw_perf_enable() but + * this way we* ensure caller won't try to enable + * individual events + */ + return 1; +}
That most certainly looses error codes for all !is_x86_event() events in the group.
So you either need to deal with that event_sched_in() failing, or guarantee that it always succeeds (forcing software events only for example).
Quoted 79 lines · Peter Zijlstra
Quoted 63 lines · Stephane Eranian
Quoted 4 lines
One concern I do have is the loss of error checking on event_sched_in()'s
event->pmu->enable(), that could be another 'hardware' PMU like breakpoints, in which case it could fail.Well,
x86_pmu_enable()does return an error code, so it is up to the upper layer to handle the error gracefully and in particular inperf_ctx_adjust_freq().(quoted diff) +55
-0+static void event_sched_in(struct perf_event *event, int cpu) +{ + event->state = PERF_EVENT_STATE_ACTIVE; + event->oncpu = cpu; + event->tstamp_running += event->ctx->time - event->tstamp_stopped; + if (is_software_event(event)) + event->pmu->enable(event); +} + +/* + * Called to enable a whole group of events. + * Returns 1 if the group was enabled, or -EAGAIN if it could not be. + * Assumes the caller has disabled interrupts and has + * frozen the PMU with hw_perf_save_disable. + * + * called with PMU disabled. If successful and return value 1, + * then guaranteed to call perf_enable() and hw_perf_enable() + */ +int hw_perf_group_sched_in(struct perf_event *leader, + struct perf_cpu_context *cpuctx, + struct perf_event_context *ctx, int cpu) +{ + struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu); + struct perf_event *sub; + int assign[X86_PMC_IDX_MAX]; + int n, n0, ret; + + n0 = cpuc->n_events; + + n = collect_events(cpuc, leader, true); + if (n < 0) + return n; + + ret = x86_schedule_events(cpuc, n, assign); + if (ret) + return ret; + + /* + * copy new assignment, now we know it is possible + * will be used by hw_perf_enable() + */ + memcpy(cpuc->assign, assign, n*sizeof(int)); + + cpuc->n_events = n; + cpuc->n_added = n - n0; + + n = 1; + event_sched_in(leader, cpu); + list_for_each_entry(sub, &leader->sibling_list, group_entry) { + if (sub->state != PERF_EVENT_STATE_OFF) { + event_sched_in(sub, cpu); + ++n; + } + } + ctx->nr_active += n;Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* + * 1 means successful and events are active + * This is not quite true because we defer + * actual activation until hw_perf_enable() but + * this way we* ensure caller won't try to enable + * individual events + */ + return 1; +}That most certainly looses error codes for all !is_x86_event() events in the group.
Quoted 4 lines
So you either need to deal with that
event_sched_in()failing, or guarantee that it always succeeds (forcing software events only for example).
True, but that one can be fixed because it is only called from hw_perf_group_sched_in() which returns an error code.
The same code would have to be fixed on PPC as well.
Quoted 1 line
Signature · 8 lines
Stephane Eranian | EMEA Software Engineering Google France | 38 avenue de l'Opéra | 75002 Paris Tel : +33 (0) 1 42 68 53 00 This email may be confidential or privileged. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it went to the wrong person. Thanks
Ok,
Something like that should problably do it:
static void event_sched_out(struct perf_event *event, int cpu)
{
event->state = PERF_EVENT_STATE_INACTIVE;
event->oncpu = -1;
}hw_perf_group_sched_in()
{
....
n = 1;
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
if (sub->state > PERF_EVENT_STATE_OFF) {
ret = event_sched_in(sub, cpu);
if (ret)
goto undo;
++n;
}
}
/*
* copy new assignment, now we know it is possible
* will be used by hw_perf_enable()
*/
memcpy(cpuc->assign, assign, n*sizeof(int));
cpuc->n_events = n;
cpuc->n_added = n - n0;
ctx->nr_active += n;
/*
* 1 means successful and events are active
* This is not quite true because we defer
* actual activation until hw_perf_enable() but
* this way we* ensure caller won't try to enable
* individual events
*/
return 1;
undo:
event_sched_out(leader, cpu);
n0 = n;
n = 1;
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
if (sub->state == PERF_EVENT_STATE_ACTIVE) {
event_sched_out(sub, cpu);
if (++n == n0)
break;
}
}
return ret;
}Looking at the generic and PPC code, there are a few points which are still unclear to me:
1. I believe that the check on attr_exclusive as is done in perf_event.c:event_sched_in()
is missing from my patch and the PPC equivalent code.
if (event->attr.exclusive)
cpuctx->exclusive = 1;
So is the smp_wmb() and the accounting in cpuctx->active_oncpu.2. the management of event->tstamp_stopped, tstamp_running
Looks like we may have to undo the update to stamp_running or
defer the update
until we know for sure, event_sched_in() succeeded for all events,
incl. software
events.
Quoted 103 lines · Stephane Eranian
Quoted 79 lines · Peter Zijlstra
Quoted 63 lines · Stephane Eranian
Quoted 4 lines
One concern I do have is the loss of error checking on event_sched_in()'s
event->pmu->enable(), that could be another 'hardware' PMU like breakpoints, in which case it could fail.Well,
x86_pmu_enable()does return an error code, so it is up to the upper layer to handle the error gracefully and in particular inperf_ctx_adjust_freq().(quoted diff) +55
-0+static void event_sched_in(struct perf_event *event, int cpu) +{ + event->state = PERF_EVENT_STATE_ACTIVE; + event->oncpu = cpu; + event->tstamp_running += event->ctx->time - event->tstamp_stopped; + if (is_software_event(event)) + event->pmu->enable(event); +} + +/* + * Called to enable a whole group of events. + * Returns 1 if the group was enabled, or -EAGAIN if it could not be. + * Assumes the caller has disabled interrupts and has + * frozen the PMU with hw_perf_save_disable. + * + * called with PMU disabled. If successful and return value 1, + * then guaranteed to call perf_enable() and hw_perf_enable() + */ +int hw_perf_group_sched_in(struct perf_event *leader, + struct perf_cpu_context *cpuctx, + struct perf_event_context *ctx, int cpu) +{ + struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu); + struct perf_event *sub; + int assign[X86_PMC_IDX_MAX]; + int n, n0, ret; + + n0 = cpuc->n_events; + + n = collect_events(cpuc, leader, true); + if (n < 0) + return n; + + ret = x86_schedule_events(cpuc, n, assign); + if (ret) + return ret; + + /* + * copy new assignment, now we know it is possible + * will be used by hw_perf_enable() + */ + memcpy(cpuc->assign, assign, n*sizeof(int)); + + cpuc->n_events = n; + cpuc->n_added = n - n0; + + n = 1; + event_sched_in(leader, cpu); + list_for_each_entry(sub, &leader->sibling_list, group_entry) { + if (sub->state != PERF_EVENT_STATE_OFF) { + event_sched_in(sub, cpu); + ++n; + } + } + ctx->nr_active += n;Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* + * 1 means successful and events are active + * This is not quite true because we defer + * actual activation until hw_perf_enable() but + * this way we* ensure caller won't try to enable + * individual events + */ + return 1; +}That most certainly looses error codes for all !is_x86_event() events in the group.
Quoted 4 lines
So you either need to deal with that
event_sched_in()failing, or guarantee that it always succeeds (forcing software events only for example).True, but that one can be fixed because it is only called from
hw_perf_group_sched_in()which returns an error code.The same code would have to be fixed on PPC as well.
Quoted 1 line
--
Stephane Eranian | EMEA Software Engineering
Google France | 38 avenue de l'Opéra | 75002 Paris
Tel : +33 (0) 1 42 68 53 00
This email may be confidential or privileged. If you received this communication by mistake, please
don't forward it to anyone else, please erase all copies and attachments, and please let me know that
it went to the wrong person. Thanks
Signature · 8 lines
Stephane Eranian | EMEA Software Engineering Google France | 38 avenue de l'Opéra | 75002 Paris Tel : +33 (0) 1 42 68 53 00 This email may be confidential or privileged. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it went to the wrong person. Thanks
Quoted 9 lines · Stephane Eranian
Ok,
Something like that should problably do it:
static void
event_sched_out(struct perf_event *event, int cpu){ event->state = PERF_EVENT_STATE_INACTIVE; event->oncpu = -1; }
You need to also call pmu->disable() if it is a software event, because a breakpoint needs to be unregistered in hardware level too.
And disable it in x86 level if it is an x86 event?
Quoted 9 lines
hw_perf_group_sched_in(){ .... n = 1; list_for_each_entry(sub, &leader->sibling_list, group_entry) { if (sub->state > PERF_EVENT_STATE_OFF) { ret = event_sched_in(sub, cpu); if (ret) goto undo;
Yeah we indeed really need to check that.
Thanks.
Frederic,
Here is what I have now in the x86 code.
As for your comment on disabling the x86 event, we don't need to do this because it is not actually activated yet when we return from hw_perf_group_sched_in(). Activation occurs really in hw_perf_enable().
static int x86_event_sched_in(struct perf_event *event,
struct perf_cpu_context *cpuctx, int cpu)
{
int ret = 0;
event->state = PERF_EVENT_STATE_ACTIVE;
event->oncpu = cpu;
event->tstamp_running += event->ctx->time - event->tstamp_stopped;
if (is_software_event(event))
ret = event->pmu->enable(event);
if (!ret && !is_software_event(event))
cpuctx->active_oncpu++;
if (!ret && event->attr.exclusive)
cpuctx->exclusive = 1;
return ret;
}static void x86_event_sched_out(struct perf_event *event,
struct perf_cpu_context *cpuctx, int cpu)
{
event->state = PERF_EVENT_STATE_INACTIVE;
event->oncpu = -1;
event->tstamp_running -= event->ctx->time - event->tstamp_stopped;
if (is_software_event(event))
event->pmu->disable(event);
if (!is_software_event(event))
cpuctx->active_oncpu--;
if (event->attr.exclusive || !cpuctx->active_oncpu)
cpuctx->exclusive = 0;
}Quoted 38 lines · Frederic Weisbecker
Quoted 9 lines · Stephane Eranian
Ok,
Something like that should problably do it:
static void
event_sched_out(struct perf_event *event, int cpu){
event->state=PERF_EVENT_STATE_INACTIVE;event->oncpu= -1;
}You need to also call
pmu->disable()if it is a software event, because a breakpoint needs to be unregistered in hardware level too.And disable it in x86 level if it is an x86 event?
Quoted 9 lines
hw_perf_group_sched_in()
{
....
n = 1;
list_for_each_entry(sub, &leader->sibling_list, group_entry){ if (sub->state>PERF_EVENT_STATE_OFF) { ret =event_sched_in(sub, cpu);
if (ret)
goto undo;Yeah we indeed really need to check that.
Thanks.
Signature · 8 lines
Stephane Eranian | EMEA Software Engineering Google France | 38 avenue de l'Opéra | 75002 Paris Tel : +33 (0) 1 42 68 53 00 This email may be confidential or privileged. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it went to the wrong person. Thanks
Quoted 9 lines · Stephane Eranian
Frederic,
Here is what I have now in the x86 code.
As for your comment on disabling the x86 event, we don't need to do this because it is not actually activated yet when we return from
hw_perf_group_sched_in(). Activation occurs really inhw_perf_enable().
Ah, indeed.
Quoted 25 lines
static int x86_event_sched_in(struct
perf_event*event,
structperf_cpu_context*cpuctx, int cpu){ int ret = 0; event->state = PERF_EVENT_STATE_ACTIVE; event->oncpu = cpu; event->tstamp_running += event->ctx->time - event->tstamp_stopped; if (is_software_event(event)) ret = event->pmu->enable(event); if (!ret && !is_software_event(event)) cpuctx->active_oncpu++; if (!ret && event->attr.exclusive) cpuctx->exclusive = 1; return ret; }static void x86_event_sched_out(struct
perf_event*event,
structperf_cpu_context*cpuctx, int cpu)
Quoted 15 lines
{
event->state = PERF_EVENT_STATE_INACTIVE;
event->oncpu = -1;
event->tstamp_running -= event->ctx->time - event->tstamp_stopped;
if (is_software_event(event))
event->pmu->disable(event);
if (!is_software_event(event))
cpuctx->active_oncpu--;
if (event->attr.exclusive || !cpuctx->active_oncpu)
cpuctx->exclusive = 0;
}Yeah looks good.
Thanks.
hi,
Will repost a new version of the patch with these changes.
Quoted 63 lines · Frederic Weisbecker
Quoted 9 lines · Stephane Eranian
Frederic,
Here is what I have now in the x86 code.
As for your comment on disabling the x86 event, we don't need to do this because it is not actually activated yet when we return from
hw_perf_group_sched_in(). Activation occurs really inhw_perf_enable().Ah, indeed.
Quoted 34 lines
static int
x86_event_sched_in(struct perf_event *event, struct perf_cpu_context *cpuctx, int cpu){
int ret = 0;
event->state=PERF_EVENT_STATE_ACTIVE;
event->oncpu= cpu;
event->tstamp_running+=event->ctx->time-event->tstamp_stopped;if (
is_software_event(event))
ret =event->pmu->enable(event);if (!ret && !is_software_event(event))
cpuctx->active_oncpu++;if (!ret &&
event->attr.exclusive)
cpuctx->exclusive= 1;return ret;
}static void
x86_event_sched_out(struct perf_event *event, struct perf_cpu_context *cpuctx, int cpu){
event->state=PERF_EVENT_STATE_INACTIVE;event->oncpu= -1;
event->tstamp_running-=event->ctx->time-event->tstamp_stopped;if (
is_software_event(event))
event->pmu->disable(event);Quoted 6 lines
if (!is_software_event(event))
cpuctx->active_oncpu--;if (
event->attr.exclusive|| !cpuctx->active_oncpu)cpuctx->exclusive= 0;
}Yeah looks good.
Thanks.
Signature · 8 lines
Stephane Eranian | EMEA Software Engineering Google France | 38 avenue de l'Opéra | 75002 Paris Tel : +33 (0) 1 42 68 53 00 This email may be confidential or privileged. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it went to the wrong person. Thanks
Peter Zijlstra
maintainerQuoted 3 lines · Frederic Weisbecker
You need to also call
pmu->disable()if it is a software event, because a breakpoint needs to be unregistered in hardware level too.
breakpoint isn't a software pmu. But yeah, enable and disable need to match.
Peter Zijlstra
maintainerPATCH 1/1 perf: fix the is_software_event() definition
Quoted 8 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
You need to also call
pmu->disable()if it is a software event, because a breakpoint needs to be unregistered in hardware level too.breakpoint isn't a software pmu. But yeah, enable and disable need to match.
That is, it shouldn't be a software pmu, because we assume software events can always be scheduled, whereas that's definitely not so for the breakpoint one.
Which seems to suggest the following
---
Subject: perf: fix the is_software_event() definition
When adding the breakpoint pmu Frederic forgot to exclude it from being a software event. While we're at it, make it an inclusive expression.
--- include/linux/perf_event.h | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)
include/linux/perf_event.h +7 -3
@@ -814,9 +814,13 @@ extern int perf_event_overflow(struct perf_event *event, int nmi,*/ static inline int is_software_event (struct perf_event *event) { -return (event->attr.type !=PERF_TYPE_RAW ) && - (event->attr.type !=PERF_TYPE_HARDWARE ) && - (event->attr.type !=PERF_TYPE_HW_CACHE ); +switch (event->attr.type) { +case PERF_TYPE_SOFTWARE : +case PERF_TYPE_TRACEPOINT : +case PERF_TYPE_HW_CACHE : +return 1 ; + } +return 0 ; }extern atomic_t perf_swevent_enabled[PERF_COUNT_SW_MAX ];
Re: [PATCH] perf: fix the is_software_event() definition
Quoted 16 lines · Peter Zijlstra
Quoted 8 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
You need to also call
pmu->disable()if it is a software event, because a breakpoint needs to be unregistered in hardware level too.breakpoint isn't a software pmu. But yeah, enable and disable need to match.
That is, it shouldn't be a software pmu, because we assume software events can always be scheduled, whereas that's definitely not so for the breakpoint one.
Which seems to suggest the following
Quoted 7 lines
---
Subject: perf: fix theis_software_event()definitionWhen adding the breakpoint pmu Frederic forgot to exclude it from being a software event. While we're at it, make it an inclusive expression.
Signed-off-by Peter Zijlstra <[email protected]>
Agreed.
But then Stephane will need to update his patch and use something else than is_software_event() to guess if an event needs its pmu->enable/disable to be called.
A kind of helper that can tell: I am not handled by hw_perf_group_sched_in()
But I suck too much in naming to propose something sane :)
Peter Zijlstra
maintainerRe: [PATCH] perf: fix the is_software_event() definition
Quoted 5 lines · Frederic Weisbecker
Agreed.
But then Stephane will need to update his patch and use something else than
is_software_event()to guess if an event needs its pmu->enable/disable to be called.
Yes, that's what I told him before and even send a patch for, the name I chose was is_x86_event().
Re: [perfmon2] [PATCH] perf: fix the is_software_event() definition
Quoted 11 lines · Peter Zijlstra
Quoted 5 lines · Frederic Weisbecker
Agreed.
But then Stephane will need to update his patch and use something else than
is_software_event()to guess if an event needs its pmu->enable/disable to be called.Yes, that's what I told him before and even send a patch for, the name I chose was
is_x86_event().
Yes, I remember that. I will modify the patch to use a function like that. I think the PPC code would need to be updated similarly.
Re: [perfmon2] [PATCH] perf: fix the is_software_event() definition
In reply to Frederic WeisbeckerQuoted 27 lines · Frederic Weisbecker
Quoted 16 lines · Peter Zijlstra
Quoted 8 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
You need to also call
pmu->disable()if it is a software event, because a breakpoint needs to be unregistered in hardware level too.breakpoint isn't a software pmu. But yeah, enable and disable need to match.
That is, it shouldn't be a software pmu, because we assume software events can always be scheduled, whereas that's definitely not so for the breakpoint one.
Which seems to suggest the following
Quoted 7 lines
---
Subject: perf: fix theis_software_event()definitionWhen adding the breakpoint pmu Frederic forgot to exclude it from being a software event. While we're at it, make it an inclusive expression.
Signed-off-by Peter Zijlstra <[email protected]>
Quoted 9 lines
Agreed.
But then Stephane will need to update his patch and use something else than
is_software_event()to guess if an event needs its pmu->enable/disable to be called.A kind of helper that can tell: I am not handled by
hw_perf_group_sched_in()
Then, we should use something that checks if the event is handled by the X86 PMU layer:
int is_x86_hw_event(struct perf_event *event)
{
return event->pmu == x86_pmu;
}
Re: [perfmon2] [PATCH] perf: fix the is_software_event() definition
2010/1/18 stephane eranian <[email protected]>:
Quoted 37 lines · Frederic Weisbecker
Quoted 27 lines · Peter Zijlstra
Quoted 16 lines · Peter Zijlstra
Quoted 8 lines · Frederic Weisbecker
Quoted 4 lines
You need to also call
pmu->disable()if it is a software event, because a breakpoint needs to be unregistered in hardware level too.breakpoint isn't a software pmu. But yeah, enable and disable need to match.
That is, it shouldn't be a software pmu, because we assume software events can always be scheduled, whereas that's definitely not so for the breakpoint one.
Which seems to suggest the following
Quoted 7 lines
---
Subject: perf: fix theis_software_event()definitionWhen adding the breakpoint pmu Frederic forgot to exclude it from being a software event. While we're at it, make it an inclusive expression.
Signed-off-by Peter Zijlstra <[email protected]>Quoted 9 lines
Agreed.
But then Stephane will need to update his patch and use something else than
is_software_event()to guess if an event needs its pmu->enable/disable to be called.A kind of helper that can tell: I am not handled by
hw_perf_group_sched_in()
Quoted 8 lines
Then, we should use something that checks if the event is handled by the X86 PMU layer:
int
is_x86_hw_event(struct perf_event *event)
{
returnevent->pmu==x86_pmu;
}
Yeah. I missed this patch from Peter in its answer. Looks good.
Re: [perfmon2] [PATCH] perf: fix the is_software_event() definition
In reply to Peter ZijlstraQuoted 39 lines · Peter Zijlstra
Quoted 8 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
You need to also call
pmu->disable()if it is a software event, because a breakpoint needs to be unregistered in hardware level too.breakpoint isn't a software pmu. But yeah, enable and disable need to match.
That is, it shouldn't be a software pmu, because we assume software events can always be scheduled, whereas that's definitely not so for the breakpoint one.
Which seems to suggest the following
---
Subject: perf: fix theis_software_event()definitionWhen adding the breakpoint pmu Frederic forgot to exclude it from being a software event. While we're at it, make it an inclusive expression.
Signed-off-by Peter Zijlstra <[email protected]>---
include/linux/perf_event.h | 10 +++++++---1 files changed, 7 insertions(+), 3 deletions(-)include/linux/perf_event.h +0
-3@@ -814,9 +814,13 @@ extern int perf_event_overflow(struct perf_event *event, int nmi,*/ static inline int is_software_event(struct perf_event *event) { - return (event->attr.type != PERF_TYPE_RAW) && - (event->attr.type != PERF_TYPE_HARDWARE) && - (event->attr.type != PERF_TYPE_HW_CACHE);
include/linux/perf_event.h extern int perf_event_overflow(struct perf_event *event, int nmi, · line 826
Quoted 10 lines
include/linux/perf_event.h +7
-0+ switch (event->attr.type) { + case PERF_TYPE_SOFTWARE: + case PERF_TYPE_TRACEPOINT: + case PERF_TYPE_HW_CACHE: + return 1; + } + return 0; } extern atomic_t perf_swevent_enabled[PERF_COUNT_SW_MAX];
PERF_TYPE_HW_CACHE is a hardware PMU event.
Quoted 13 lines
------------------------------------------------------------------------------
Throughout its 18-year history, RSA Conference consistently attracts the world's best and brightest in the field, creating opportunities for Conference attendees to learn about information security's most important issues through interactions with peers, luminaries and emerging and established companies.
http://p.sf.net/sfu/rsaconf-dev2dev
_______________________________________________
perfmon2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
Peter Zijlstra
maintainerRe: [perfmon2] [PATCH] perf: fix the is_software_event() definition
Quoted 1 line · stephane eranian
PERF_TYPE_HW_CACHEis a hardware PMU event.
D'0h!