perf/x86/intel: Add proper condition to run sched_task callbacks
Subsystems (2)
Jiri Olsa
reviewerPATCH 1/1 perf/x86/intel: Add proper condition to run sched_task callbacks
The x86 pmu currently uses the sched_task callback for 2 functions:
- PEBS drain
- save/restore LBR data
They are both triggered once the x86 pmu is registered withperf_sched_cb_inc call (within pmu::add callback), regardless
if there's actually any PEBS or LBR event configured on the cpu.
This can lead to extra cycles in some perf monitoring, like when we monitor PEBS event without LBR data. We need PEBS, non freq/timestamp event to enable the sched_task callback:
# perf record --no-timestamp -c 10000 -e cycles:p ./perf bench sched pipe -l 1000000
The perf stat with cycles and msr:write_msr if above command before:
...
Performance counter stats for './perf record --no-timestamp -c 10000 -e cycles:p \
./perf bench sched pipe -l 1000000' (5 runs):
18,519,557,441 cycles:k
91,195,527 msr:write_msr
29.334476406 seconds time elapsedAnd after the change:
...
Performance counter stats for './perf record --no-timestamp -c 10000 -e cycles:p \
./perf bench sched pipe -l 1000000' (5 runs):
18,565,757,840 cycles:k
27,103,160 msr:write_msr
16.253026030 seconds time elapsedThere's no affect on cycles:k because the sched_task happens with events switched off, however the msr:write_msr tracepoint counter and almost 50% of time speedup show the improvement.
Monitoring LBR event and having extra PEBS drain processing in sched_task callback showed just a little speedup, because the drain function does not do much extra work in case there is no PEBS data.
Fixing this by adding PEBS and LBR conditions for relevant event data being configured on cpu into intel_pmu_sched_task callback.
--- arch/x86/events/intel/core.c | 6 ++++-- arch/x86/events/intel/ds.c | 8 ++++---- arch/x86/events/perf_event.h | 2 ++ 3 files changed, 10 insertions(+), 6 deletions(-)
arch/x86/events/intel/core.c +4 -2
@@ -3265,9 +3265,11 @@ static void intel_pmu_cpu_dying(int cpu)static void intel_pmu_sched_task (struct perf_event_context *ctx,bool sched_in) { -if (x86_pmu.pebs_active) +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +if (intel_pmu_pebs_needs_sched_cb (cpuc))intel_pmu_pebs_sched_task (ctx, sched_in); -if (x86_pmu.lbr_nr) +if (cpuc->lbr_users)intel_pmu_lbr_sched_task (ctx, sched_in); }
arch/x86/events/intel/ds.c +4 -4
@@ -811,7 +811,7 @@ struct event_constraint *intel_pebs_constraints(struct perf_event *event)* the large interrupt threshold, such that we can provide PID and TID * to PEBS samples. */ -static inline bool pebs_needs_sched_cb (struct cpu_hw_events *cpuc) +inline bool intel_pmu_pebs_needs_sched_cb (struct cpu_hw_events *cpuc) {return cpuc->n_pebs && (cpuc->n_pebs == cpuc->n_large_pebs); }
@@ -841,7 +841,7 @@ pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc, struct pmu *pmu)*/ bool update = cpuc->n_pebs ==1 ; -if (needed_cb !=pebs_needs_sched_cb (cpuc)) { +if (needed_cb !=intel_pmu_pebs_needs_sched_cb (cpuc)) {if (!needed_cb)perf_sched_cb_inc (pmu);else
@@ -858,7 +858,7 @@ void intel_pmu_pebs_add(struct perf_event *event) {struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events);struct hw_perf_event *hwc = &event->hw; -bool needed_cb =pebs_needs_sched_cb (cpuc); +bool needed_cb =intel_pmu_pebs_needs_sched_cb (cpuc); cpuc->n_pebs++;if (hwc->flags &PERF_X86_EVENT_FREERUNNING )
@@ -896,7 +896,7 @@ void intel_pmu_pebs_del(struct perf_event *event) {struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events);struct hw_perf_event *hwc = &event->hw; -bool needed_cb =pebs_needs_sched_cb (cpuc); +bool needed_cb =intel_pmu_pebs_needs_sched_cb (cpuc); cpuc->n_pebs--;if (hwc->flags &PERF_X86_EVENT_FREERUNNING )
arch/x86/events/perf_event.h +2 -0
@@ -909,6 +909,8 @@ void intel_pmu_pebs_disable_all(void);void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in); +bool intel_pmu_pebs_needs_sched_cb (struct cpu_hw_events *cpuc); +void intel_ds_init (void );void intel_pmu_lbr_sched_task (struct perf_event_context *ctx,bool sched_in);
Signature · 1 line
2.9.4
Peter Zijlstra
maintainerQuoted 7 lines · Jiri Olsa
The x86 pmu currently uses the
sched_taskcallback for 2 functions:
- PEBS drain
- save/restore LBR dataThey are both triggered once the x86 pmu is registered with
perf_sched_cb_inccall (withinpmu::addcallback), regardless
if there's actually any PEBS or LBR event configured on the cpu.
I don't understand. If we do pmu::add() we _are_ on the CPU.
So you're saying intel_pmu_pebs_{add,del}() are doing it wrong? So why not fix those?
arch/x86/events/intel/core.c static void intel_pmu_cpu_dying(int cpu) · line 3271
Quoted 13 lines
arch/x86/events/intel/core.c +3
-1@@ -3265,9 +3265,11 @@ static void intel_pmu_cpu_dying(int cpu)static void intel_pmu_sched_task (struct perf_event_context *ctx,bool sched_in) { -if (x86_pmu.pebs_active) +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +if (intel_pmu_pebs_needs_sched_cb (cpuc))intel_pmu_pebs_sched_task (ctx, sched_in);
So I'm confused, if we'd not need this, how come we're here in the first place?
Quoted 14 lines · Peter Zijlstra
Quoted 7 lines · Jiri Olsa
The x86 pmu currently uses the
sched_taskcallback for 2 functions:
- PEBS drain
- save/restore LBR dataThey are both triggered once the x86 pmu is registered with
perf_sched_cb_inccall (withinpmu::addcallback), regardless
if there's actually any PEBS or LBR event configured on the cpu.I don't understand. If we do
pmu::add()we _are_ on the CPU.So you're saying intel_pmu_pebs_{add,del}() are doing it wrong? So why not fix those?
Quoted 17 lines
arch/x86/events/intel/core.c static void intel_pmu_cpu_dying(int cpu) · line 3271
Quoted 13 lines
arch/x86/events/intel/core.c +3
-1@@ -3265,9 +3265,11 @@ static void intel_pmu_cpu_dying(int cpu)static void intel_pmu_sched_task (struct perf_event_context *ctx,bool sched_in) { -if (x86_pmu.pebs_active) +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +if (intel_pmu_pebs_needs_sched_cb (cpuc))intel_pmu_pebs_sched_task (ctx, sched_in);So I'm confused, if we'd not need this, how come we're here in the first place?
because we have 2 places using the same callback
- PEBS drain for free running counters
- LBR save/store
both of them called from intel_pmu_sched_task
so let's say PEBS drain setup the callback for the event, but in the callback itself (intel_pmu_sched_task) we will also run the code for LBR save/restore, which we did not ask for, but the code in intel_pmu_sched_task does not check for that
I'm adding conditions to recognize the work that needs to be done in the callback
another option might be to add support for more x86_pmu::sched_task callbacks, which might be cleaner
jirka
Peter Zijlstra
maintainerQuoted 11 lines · Jiri Olsa
because we have 2 places using the same callback
- PEBS drain for free running counters
- LBR save/storeboth of them called from
intel_pmu_sched_taskso let's say PEBS drain setup the callback for the event, but in the callback itself (
intel_pmu_sched_task) we will also run the code for LBR save/restore, which we did not ask for, but the code inintel_pmu_sched_taskdoes not check for that
Ah fair enough; Changelog confused me.
Quoted 5 lines
I'm adding conditions to recognize the work that needs to be done in the callback
another option might be to add support for more
x86_pmu::sched_taskcallbacks, which might be cleaner
Right; either that or pull the condition into the functions themselves to create less churn. Something like so I suppose...
arch/x86/events/intel/core.c +2 -4
@@ -3265,10 +3265,8 @@ static void intel_pmu_cpu_dying(int cpu)static void intel_pmu_sched_task (struct perf_event_context *ctx,bool sched_in) { -if (x86_pmu.pebs_active) -intel_pmu_pebs_sched_task (ctx, sched_in); -if (x86_pmu.lbr_nr) -intel_pmu_lbr_sched_task (ctx, sched_in); +intel_pmu_pebs_sched_task (ctx, sched_in); +intel_pmu_lbr_sched_task (ctx, sched_in); }PMU_FORMAT_ATTR (offcore_rsp,"config1:0-63" );
arch/x86/events/intel/ds.c +8 -6
@@ -606,12 +606,6 @@ static inline void intel_pmu_drain_pebs_buffer(void) x86_pmu.drain_pebs (®s); } -void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) -{ -if (!sched_in) -intel_pmu_drain_pebs_buffer (); -} -/* * PEBS */
@@ -816,6 +810,14 @@ static inline bool pebs_needs_sched_cb(struct cpu_hw_events *cpuc)return cpuc->n_pebs && (cpuc->n_pebs == cpuc->n_large_pebs); } +void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) +{ +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +if (!sched_in &&pebs_needs_sched_cb (cpuc)) +intel_pmu_drain_pebs_buffer (); +} +static inline void pebs_update_threshold (struct cpu_hw_events *cpuc) {struct debug_store *ds = cpuc->ds;
arch/x86/events/intel/lbr.c +4 -0
@@ -380,8 +380,12 @@ static void __intel_pmu_lbr_save(struct x86_perf_task_context *task_ctx)void intel_pmu_lbr_sched_task (struct perf_event_context *ctx,bool sched_in) { +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events);struct x86_perf_task_context *task_ctx; +if (!cpuc->lbr_users) +return ; +/* * If LBR callstack feature is enabled and the stack was saved when * the task was scheduled out, restore the stack. Otherwise flush
Quoted 16 lines · Peter Zijlstra
Quoted 11 lines · Jiri Olsa
because we have 2 places using the same callback
- PEBS drain for free running counters
- LBR save/storeboth of them called from
intel_pmu_sched_taskso let's say PEBS drain setup the callback for the event, but in the callback itself (
intel_pmu_sched_task) we will also run the code for LBR save/restore, which we did not ask for, but the code inintel_pmu_sched_taskdoes not check for thatAh fair enough; Changelog confused me.
Quoted 8 lines
Quoted 5 lines
I'm adding conditions to recognize the work that needs to be done in the callback
another option might be to add support for more
x86_pmu::sched_taskcallbacks, which might be cleanerRight; either that or pull the condition into the functions themselves to create less churn. Something like so I suppose...
ok, will send new version
jirka
Quoted 68 lines
arch/x86/events/intel/core.c +2
-4@@ -3265,10 +3265,8 @@ static void intel_pmu_cpu_dying(int cpu)static void intel_pmu_sched_task (struct perf_event_context *ctx,bool sched_in) { -if (x86_pmu.pebs_active) -intel_pmu_pebs_sched_task (ctx, sched_in); -if (x86_pmu.lbr_nr) -intel_pmu_lbr_sched_task (ctx, sched_in); +intel_pmu_pebs_sched_task (ctx, sched_in); +intel_pmu_lbr_sched_task (ctx, sched_in); }PMU_FORMAT_ATTR (offcore_rsp,"config1:0-63" );arch/x86/events/intel/ds.c +8
-6@@ -606,12 +606,6 @@ static inline void intel_pmu_drain_pebs_buffer(void) x86_pmu.drain_pebs (®s); } -void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) -{ -if (!sched_in) -intel_pmu_drain_pebs_buffer (); -} -/* * PEBS */ @@ -816,6 +810,14 @@ static inline bool pebs_needs_sched_cb(struct cpu_hw_events *cpuc)return cpuc->n_pebs && (cpuc->n_pebs == cpuc->n_large_pebs); } +void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) +{ +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +if (!sched_in &&pebs_needs_sched_cb (cpuc)) +intel_pmu_drain_pebs_buffer (); +} +static inline void pebs_update_threshold (struct cpu_hw_events *cpuc) {struct debug_store *ds = cpuc->ds;arch/x86/events/intel/lbr.c +4
-0@@ -380,8 +380,12 @@ static void __intel_pmu_lbr_save(struct x86_perf_task_context *task_ctx)void intel_pmu_lbr_sched_task (struct perf_event_context *ctx,bool sched_in) { +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events);struct x86_perf_task_context *task_ctx; +if (!cpuc->lbr_users) +return ; +/* * If LBR callstack feature is enabled and the stack was saved when * the task was scheduled out, restore the stack. Otherwise flush
Quoted 8 lines · Peter Zijlstra
Quoted 6 lines · Jiri Olsa
because we have 2 places using the same callback
- PEBS drain for free running counters
- LBR save/storeboth of them called from
intel_pmu_sched_task
Quoted 7 lines
Quoted 5 lines
so let's say PEBS drain setup the callback for the event, but in the callback itself (
intel_pmu_sched_task) we will also run the code for LBR save/restore, which we did not ask for, but the code inintel_pmu_sched_taskdoes not check for thatAh fair enough; Changelog confused me.
just when I thought that my english is getting bedder ;-)
v2 attached with new changelog
jirka
---
We have 2 functions using the same sched_task callback:
- PEBS drain for free running counters
- LBR save/store
Both of them are called from intel_pmu_sched_task and either of them can be unwillingly triggered when the other one is configured to run.
Let's say there's PEBS drain configured in sched_task callback for the event, but in the callback itself (intel_pmu_sched_task) we will also run the code for LBR save/restore, which we did not ask for, but the code in intel_pmu_sched_task does not check for that.
This can lead to extra cycles in some perf monitoring, like when we monitor PEBS event without LBR data.
# perf record --no-timestamp -c 10000 -e cycles:p ./perf bench sched pipe -l 1000000
(We need PEBS, non freq/non timestamp event to enable
the sched_task callback)
The perf stat of cycles and msr:write_msr for above command before the change:
...
Performance counter stats for './perf record --no-timestamp -c 10000 -e cycles:p \
./perf bench sched pipe -l 1000000' (5 runs):
18,519,557,441 cycles:k
91,195,527 msr:write_msr
29.334476406 seconds time elapsedAnd after the change:
...
Performance counter stats for './perf record --no-timestamp -c 10000 -e cycles:p \
./perf bench sched pipe -l 1000000' (5 runs):
18,704,973,540 cycles:k
27,184,720 msr:write_msr
16.977875900 seconds time elapsedThere's no affect on cycles:k because the sched_task happens with events switched off, however the msr:write_msr tracepoint counter together with almost 50% of time speedup show the improvement.
Monitoring LBR event and having extra PEBS drain processing in sched_task callback showed just a little speedup, because the drain function does not do much extra work in case there is no PEBS data.
Adding conditions to recognize the configured work that needs to be done in the x86_pmu's sched_task callback.
--- arch/x86/events/intel/core.c | 6 ++---- arch/x86/events/intel/ds.c | 14 ++++++++------ arch/x86/events/intel/lbr.c | 4 ++++ 3 files changed, 14 insertions(+), 10 deletions(-)
arch/x86/events/intel/core.c +2 -4
@@ -3397,10 +3397,8 @@ static void intel_pmu_cpu_dying(int cpu)static void intel_pmu_sched_task (struct perf_event_context *ctx,bool sched_in) { -if (x86_pmu.pebs_active) -intel_pmu_pebs_sched_task (ctx, sched_in); -if (x86_pmu.lbr_nr) -intel_pmu_lbr_sched_task (ctx, sched_in); +intel_pmu_pebs_sched_task (ctx, sched_in); +intel_pmu_lbr_sched_task (ctx, sched_in); }PMU_FORMAT_ATTR (offcore_rsp,"config1:0-63" );
arch/x86/events/intel/ds.c +8 -6
@@ -606,12 +606,6 @@ static inline void intel_pmu_drain_pebs_buffer(void) x86_pmu.drain_pebs (®s); } -void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) -{ -if (!sched_in) -intel_pmu_drain_pebs_buffer (); -} -/* * PEBS */
@@ -822,6 +816,14 @@ static inline bool pebs_needs_sched_cb(struct cpu_hw_events *cpuc)return cpuc->n_pebs && (cpuc->n_pebs == cpuc->n_large_pebs); } +void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) +{ +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +if (!sched_in &&pebs_needs_sched_cb (cpuc)) +intel_pmu_drain_pebs_buffer (); +} +static inline void pebs_update_threshold (struct cpu_hw_events *cpuc) {struct debug_store *ds = cpuc->ds;
arch/x86/events/intel/lbr.c +4 -0
@@ -380,8 +380,12 @@ static void __intel_pmu_lbr_save(struct x86_perf_task_context *task_ctx)void intel_pmu_lbr_sched_task (struct perf_event_context *ctx,bool sched_in) { +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events);struct x86_perf_task_context *task_ctx; +if (!cpuc->lbr_users) +return ; +/* * If LBR callstack feature is enabled and the stack was saved when * the task was scheduled out, restore the stack. Otherwise flush
Signature · 1 line
2.9.4
tip-bot for Jiri Olsa
merge botperf/x86/intel: Add proper condition to run sched_task callbacks
Commit-ID: df6c3db8d30fb1699ccbc403196b86324f4257af Gitweb: http://git.kernel.org/tip/df6c3db8d30fb1699ccbc403196b86324f4257af Author: Jiri Olsa <[email protected]> AuthorDate: Wed, 19 Jul 2017 09:52:47 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 21 Jul 2017 09:58:39 +0200
perf/x86/intel: Add proper condition to run sched_task callbacks
We have 2 functions using the same sched_task callback:
- PEBS drain for free running counters
- LBR save/store
Both of them are called from intel_pmu_sched_task() and either of them can be unwillingly triggered when the other one is configured to run.
Let's say there's PEBS drain configured in sched_task callback for the event, but in the callback itself (intel_pmu_sched_task()) we will also run the code for LBR save/restore, which we did not ask for, but the code in intel_pmu_sched_task() does not check for that.
This can lead to extra cycles in some perf monitoring, like when we monitor PEBS event without LBR data.
# perf record --no-timestamp -c 10000 -e cycles:p ./perf bench sched pipe -l 1000000
(We need PEBS, non freq/non timestamp event to enable
the sched_task callback)
The perf stat of cycles and msr:write_msr for above command before the change:
...
Performance counter stats for './perf record --no-timestamp -c 10000 -e cycles:p \
./perf bench sched pipe -l 1000000' (5 runs):
18,519,557,441 cycles:k
91,195,527 msr:write_msr
29.334476406 seconds time elapsedAnd after the change:
...
Performance counter stats for './perf record --no-timestamp -c 10000 -e cycles:p \
./perf bench sched pipe -l 1000000' (5 runs):
18,704,973,540 cycles:k
27,184,720 msr:write_msr
16.977875900 seconds time elapsedThere's no affect on cycles:k because the sched_task happens with events switched off, however the msr:write_msr tracepoint counter together with almost 50% of time speedup show the improvement.
Monitoring LBR event and having extra PEBS drain processing in sched_task callback showed just a little speedup, because the drain function does not do much extra work in case there is no PEBS data.
Adding conditions to recognize the configured work that needs to be done in the x86_pmu's sched_task callback.
--- arch/x86/events/intel/core.c | 6 ++---- arch/x86/events/intel/ds.c | 14 ++++++++------ arch/x86/events/intel/lbr.c | 4 ++++ 3 files changed, 14 insertions(+), 10 deletions(-)
arch/x86/events/intel/core.c +2 -4
@@ -3397,10 +3397,8 @@ static void intel_pmu_cpu_dying(int cpu)static void intel_pmu_sched_task (struct perf_event_context *ctx,bool sched_in) { -if (x86_pmu.pebs_active) -intel_pmu_pebs_sched_task (ctx, sched_in); -if (x86_pmu.lbr_nr) -intel_pmu_lbr_sched_task (ctx, sched_in); +intel_pmu_pebs_sched_task (ctx, sched_in); +intel_pmu_lbr_sched_task (ctx, sched_in); }PMU_FORMAT_ATTR (offcore_rsp,"config1:0-63" );
arch/x86/events/intel/ds.c +8 -6
@@ -606,12 +606,6 @@ static inline void intel_pmu_drain_pebs_buffer(void) x86_pmu.drain_pebs (®s); } -void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) -{ -if (!sched_in) -intel_pmu_drain_pebs_buffer (); -} -/* * PEBS */
@@ -822,6 +816,14 @@ static inline bool pebs_needs_sched_cb(struct cpu_hw_events *cpuc)return cpuc->n_pebs && (cpuc->n_pebs == cpuc->n_large_pebs); } +void intel_pmu_pebs_sched_task (struct perf_event_context *ctx,bool sched_in) +{ +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events); + +if (!sched_in &&pebs_needs_sched_cb (cpuc)) +intel_pmu_drain_pebs_buffer (); +} +static inline void pebs_update_threshold (struct cpu_hw_events *cpuc) {struct debug_store *ds = cpuc->ds;
arch/x86/events/intel/lbr.c +4 -0
@@ -380,8 +380,12 @@ static void __intel_pmu_lbr_save(struct x86_perf_task_context *task_ctx)void intel_pmu_lbr_sched_task (struct perf_event_context *ctx,bool sched_in) { +struct cpu_hw_events *cpuc =this_cpu_ptr (&cpu_hw_events);struct x86_perf_task_context *task_ctx; +if (!cpuc->lbr_users) +return ; +/* * If LBR callstack feature is enabled and the stack was saved when * the task was scheduled out, restore the stack. Otherwise flush