Restore Intel LBR call stack from cloned inactive task perf context on
a context switch. This change inherently addresses inconsistency in LBR
call stack data provided on a sample in record profiling mode:
$ perf record -N -B -T -R --call-graph lbr \
-e cpu/period=0xcdfe60,event=0x3c,name=\'CPU_CLK_UNHALTED.THREAD\'/Duk \
--clockid=monotonic_raw -- ./miniFE.x nx 25 ny 25 nz 25
Let's assume threads A, B, C belonging to the same process.
B and C are siblings of A and their perf contexts are treated as equivalent.
At some point B blocks on a futex (non preempt context switch).
B's LBRs are preserved at B's perf context task_ctx_data and B's events
are removed from PMU and disabled. B's perf context becomes inactive.
Later C gets on a cpu, runs, gets profiled and eventually switches to
the awaken but not yet running B. The optimized context switch path is
executed swapping B's and C's task_ctx_data pointers at perf event contexts.
So C's task_ctx_data will refer preserved B's LBRs on the following
switch-in event.
However, as far B's perf context is inactive there is no enabled events
in there and B's task_ctx_data->lbr_callstack_users is equal to 0.
When B gets on the cpu B's events reviving is skipped following
the optimized context switch path and B's task_ctx_data->lbr_callstack_users
remains 0. Thus B's LBR's are not restored by pmu sched_task() code called
in the end of perf context switch-in callback for B.
In the report that manifests as having short fragments of B's
call stack, still tracked by LBR's HW between adjacent samples,
but the whole thread call tree doesn't aggregate.
The fix has been evaluated when profiling miniFE [1] (C++, OpenMP)
workload running 64 threads on Intel Skylake EP(64 core, 2 sockets):
$ perf report --call-graph callee,flat
5.3.0-rc6+ (tip perf/core) - fixed
- 92.66% 82.64% miniFE.x libiomp5.so [.] _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
- 69.14% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_fork_barrier
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
start_thread
__clone
- 21.89% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_barrier
__kmpc_reduce_nowait
miniFE::cg_solve<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, int>, miniFE::matvec_std<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, in
__kmp_invoke_microtask
__kmp_invoke_task_func
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
start_thread
__clone
- 1.63% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_barrier
__kmpc_reduce_nowait
main
__kmp_invoke_microtask
__kmp_invoke_task_func
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
start_thread
__clone
5.0.13-300.fc30.x86_64 - no fix
- 90.29% 81.01% miniFE.x libiomp5.so [.] _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
- 33.45% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_fork_barrier
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
start_thread
__clone
87.63% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
- 54.79% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_fork_barrier
__kmp_launch_thread
- 9.18% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_barrier
__kmpc_reduce_nowait
miniFE::cg_solve<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, int>, miniFE::matvec_std<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, in
__kmp_invoke_microtask
__kmp_invoke_task_func
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
start_thread
__clone
- 41.28% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_fork_barrier
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
- 15.77% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_barrier
__kmpc_reduce_nowait
miniFE::cg_solve<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, int>, miniFE::matvec_std<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, in
__kmp_invoke_microtask
__kmp_invoke_task_func
__kmp_launch_thread
- 11.56% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_barrier
__kmpc_reduce_nowait
miniFE::cg_solve<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, int>, miniFE::matvec_std<miniFE::CSRMatrix<double, int, int>, miniFE::Vector<double, int, in
__kmp_invoke_microtask
__kmp_invoke_task_func
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
- 2.33% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_release
__kmp_barrier
__kmpc_reduce_nowait
main
__kmp_invoke_microtask
__kmp_invoke_task_func
__kmp_launch_thread
_INTERNAL_24_______src_z_Linux_util_c_3e0095e6::__kmp_launch_worker
start_thread
__clone
0.67% _INTERNAL_25_______src_kmp_barrier_cpp_1d20fae8::__kmp_hyper_barrier_gather
0.57% __kmp_hardware_timestamp
[1] https://www.hpcadvisorycouncil.com/pdf/miniFE_Analysis_and_Profiling.pdf
---
Alexey Budankov (4):
perf/core,x86: introduce sync_task_ctx() method at struct pmu
perf/x86: install platform specific sync_task_ctx adapter
perf/x86/intel: implement LBR callstacks context synchronization
perf/core,x86: synchronize PMU task contexts on optimized context
switches
arch/x86/events/core.c | 7 +++++++
arch/x86/events/intel/core.c | 7 +++++++
arch/x86/events/intel/lbr.c | 6 ++++++
arch/x86/events/perf_event.h | 11 +++++++++++
include/linux/perf_event.h | 7 +++++++
kernel/events/core.c | 13 +++++++++++++
6 files changed, 51 insertions(+)
---
Changes in v4:
- moved check on simultaneous task_ctx_data objects availability
to the perf/core layer;
- marked sync_task_ctx() as the optional in code comments;
- renamed params of sync_task_ctx() to prev and next;
Changes in v3:
- replaced assignment with swap at intel_pmu_lbr_sync_task_ctx()
Changes in v2:
- implemented sync_task_ctx() method at perf,x86,intel pmu types;
- employed the method on the optimized context switch path between
equivalent perf event contexts;
--
2.20.1
Declare sync_task_ctx() methods at the generic and x86 specific
pmu types to bridge calls to platform specific pmu code on optimized
context switch path between equivalent task perf event contexts.
Signed-off-by: Alexey Budankov <[email protected]>
---
Changes in v4:
- marked sync_task_ctx() as the optional in code comments;
- renamed params of sync_task_ctx() to prev and next;
---
arch/x86/events/perf_event.h | 8 ++++++++
include/linux/perf_event.h | 7 +++++++
2 files changed, 15 insertions(+)
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index ecacfbf4ebc1..be78f2765f74 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -682,6 +682,14 @@ struct x86_pmu {
*/
atomic_t lbr_exclusive[x86_lbr_exclusive_max];
+ /*
+ * perf task context (i.e. struct perf_event_context::task_ctx_data) switch helper
+ * to bridge calls from perf/core to perf/x86. See struct pmu::sync_task_ctx() usage
+ * for examples;
+ */
+ void (*sync_task_ctx)(struct x86_perf_task_context *prev,
+ struct x86_perf_task_context *next);
+
/*
* AMD bits
*/
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 587ae4d002f5..4b96b4a5ac03 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -410,6 +410,13 @@ struct pmu {
*/
size_t task_ctx_size;
+ /*
+ * PMU specific parts of task perf event context (i.e. ctx->task_ctx_data)
+ * can be synchronized using this function. See Intel LBR callstack support
+ * implementation and Perf core context switch handling callbacks for usage
+ * examples.
+ */
+ void (*sync_task_ctx) (void *prev, void *next); /* optional */
/*
* Set up pmu-private data structures for an AUX area
--
2.20.1
Implement intel_pmu_lbr_sync_task_ctx() method updating counters
of the events that requested LBR callstack data on a sample.
The counter can be zero for the case when task context belongs to
a thread that has just come from a block on a futex and the context
contains saved (lbr_stack_state == LBR_VALID) LBR register values.
For the values to be restored at LBR registers on the next thread's
switch-in event it swaps the counter value with the one that is
expected to be non zero at the previous equivalent task perf event
context.
Swap operation type ensures the previous task perf event context
stays consistent with the amount of events that requested LBR
callstack data on a sample.
Signed-off-by: Alexey Budankov <[email protected]>
---
arch/x86/events/intel/lbr.c | 6 ++++++
arch/x86/events/perf_event.h | 3 +++
2 files changed, 9 insertions(+)
diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index ea54634eabf3..3cf58bcb88af 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -417,6 +417,12 @@ static void __intel_pmu_lbr_save(struct x86_perf_task_context *task_ctx)
cpuc->last_log_id = ++task_ctx->log_id;
}
+void intel_pmu_lbr_sync_task_ctx(struct x86_perf_task_context *prev,
+ struct x86_perf_task_context *next)
+{
+ swap(prev->lbr_callstack_users, next->lbr_callstack_users);
+}
+
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);
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index be78f2765f74..0474ec6f4771 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -1024,6 +1024,9 @@ void intel_pmu_store_pebs_lbrs(struct pebs_lbr *lbr);
void intel_ds_init(void);
+void intel_pmu_lbr_sync_task_ctx(struct x86_perf_task_context *prev,
+ struct x86_perf_task_context *next);
+
void intel_pmu_lbr_sched_task(struct perf_event_context *ctx, bool sched_in);
u64 lbr_from_signext_quirk_wr(u64 val);
--
2.20.1
Bridge perf core and x86 sync_task_ctx() method calls.
Signed-off-by: Alexey Budankov <[email protected]>
---
arch/x86/events/core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 7b21455d7504..f51bddf5f48c 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2243,6 +2243,12 @@ static void x86_pmu_sched_task(struct perf_event_context *ctx, bool sched_in)
x86_pmu.sched_task(ctx, sched_in);
}
+static void x86_pmu_sync_task_ctx(void *prev, void *next)
+{
+ if (x86_pmu.sync_task_ctx)
+ x86_pmu.sync_task_ctx(prev, next);
+}
+
void perf_check_microcode(void)
{
if (x86_pmu.check_microcode)
@@ -2297,6 +2303,7 @@ static struct pmu pmu = {
.event_idx = x86_pmu_event_idx,
.sched_task = x86_pmu_sched_task,
.task_ctx_size = sizeof(struct x86_perf_task_context),
+ .sync_task_ctx = x86_pmu_sync_task_ctx,
.check_period = x86_pmu_check_period,
.aux_output_match = x86_pmu_aux_output_match,
--
2.20.1
Install Intel specific PMU task context synchronization adapter and
extend optimized context switch path with PMU specific task context
synchronization to fix LBR callstack virtualization on context switches.
Signed-off-by: Alexey Budankov <[email protected]>
---
arch/x86/events/intel/core.c | 7 +++++++
kernel/events/core.c | 13 +++++++++++++
2 files changed, 20 insertions(+)
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index bbf6588d47ee..b9f518aa478e 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3820,6 +3820,12 @@ static void intel_pmu_sched_task(struct perf_event_context *ctx,
intel_pmu_lbr_sched_task(ctx, sched_in);
}
+static void intel_pmu_sync_task_ctx(struct x86_perf_task_context *one,
+ struct x86_perf_task_context *another)
+{
+ intel_pmu_lbr_sync_task_ctx(one, another);
+}
+
static int intel_pmu_check_period(struct perf_event *event, u64 value)
{
return intel_pmu_has_bts_period(event, value) ? -EINVAL : 0;
@@ -3955,6 +3961,7 @@ static __initconst const struct x86_pmu intel_pmu = {
.guest_get_msrs = intel_guest_get_msrs,
.sched_task = intel_pmu_sched_task,
+ .sync_task_ctx = intel_pmu_sync_task_ctx,
.check_period = intel_pmu_check_period,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f9a5d4356562..51d4138b06f7 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3204,11 +3204,24 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
raw_spin_lock(&ctx->lock);
raw_spin_lock_nested(&next_ctx->lock, SINGLE_DEPTH_NESTING);
if (context_equiv(ctx, next_ctx)) {
+ struct pmu *pmu = ctx->pmu;
+
WRITE_ONCE(ctx->task, next);
WRITE_ONCE(next_ctx->task, task);
swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
+ /*
+ * PMU specific parts of task perf context can require
+ * additional synchronization which makes sense only if
+ * both next_ctx->task_ctx_data and ctx->task_ctx_data
+ * pointers are allocated. As an example of such
+ * synchronization see implementation details of Intel
+ * LBR call stack data profiling;
+ */
+ if (ctx->task_ctx_data && next_ctx->task_ctx_data)
+ pmu->sync_task_ctx(next_ctx->task_ctx_data,
+ ctx->task_ctx_data);
/*
* RCU_INIT_POINTER here is safe because we've not
* modified the ctx and the above modification of
--
2.20.1
On Tue, Oct 22, 2019 at 09:01:11AM +0300, Alexey Budankov wrote:
> swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
>
> + /*
> + * PMU specific parts of task perf context can require
> + * additional synchronization which makes sense only if
> + * both next_ctx->task_ctx_data and ctx->task_ctx_data
> + * pointers are allocated. As an example of such
> + * synchronization see implementation details of Intel
> + * LBR call stack data profiling;
> + */
> + if (ctx->task_ctx_data && next_ctx->task_ctx_data)
> + pmu->sync_task_ctx(next_ctx->task_ctx_data,
> + ctx->task_ctx_data);
This still does not check if pmu->sync_task_ctx is set. If any other
arch ever uses task_ctx_data without then also supplying this method
things will go *bang*.
Also, I think I prefer the variant I gave you yesterday:
https://lkml.kernel.org/r/[email protected]
if (pmu->swap_task_ctx)
pmu->swap_task_ctx(ctx, next_ctx);
else
swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
That also unconfuses the argument order in your above patch (where you
have to undo thw swap).
Alternatively, since there currently is no other arch using
task_ctx_data, we can make the pmu::swap_task_ctx() thing mandatory when
having it and completely replace the swap(), write it like so:
- swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
+ if (pmu->swap_task_ctx)
+ pmu->swap_task_ctx(ctx, next_ctx);
Hmm?
On 22.10.2019 12:43, Peter Zijlstra wrote:
> On Tue, Oct 22, 2019 at 09:01:11AM +0300, Alexey Budankov wrote:
>
>> swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
>>
>> + /*
>> + * PMU specific parts of task perf context can require
>> + * additional synchronization which makes sense only if
>> + * both next_ctx->task_ctx_data and ctx->task_ctx_data
>> + * pointers are allocated. As an example of such
>> + * synchronization see implementation details of Intel
>> + * LBR call stack data profiling;
>> + */
>> + if (ctx->task_ctx_data && next_ctx->task_ctx_data)
>> + pmu->sync_task_ctx(next_ctx->task_ctx_data,
>> + ctx->task_ctx_data);
>
> This still does not check if pmu->sync_task_ctx is set. If any other
> arch ever uses task_ctx_data without then also supplying this method
> things will go *bang*.
Argh, and that is why it is documented as the optional one.
Undoubtedly, we have to avoid crashes on other architectures.
So "if (pmu->sync_task_ctx)" has to be a part of v5.
>
> Also, I think I prefer the variant I gave you yesterday:
>
> https://lkml.kernel.org/r/[email protected]
>
> if (pmu->swap_task_ctx)
> pmu->swap_task_ctx(ctx, next_ctx);
> else
> swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
>
> That also unconfuses the argument order in your above patch (where you
> have to undo thw swap).
I would name the method sync_task_ctx not swap_task_ctx because sync reserves
broader meaning, IMHO.
~Alexey
On 22.10.2019 12:43, Peter Zijlstra wrote:
> On Tue, Oct 22, 2019 at 09:01:11AM +0300, Alexey Budankov wrote:
>
>> swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
>>
>> + /*
>> + * PMU specific parts of task perf context can require
>> + * additional synchronization which makes sense only if
>> + * both next_ctx->task_ctx_data and ctx->task_ctx_data
>> + * pointers are allocated. As an example of such
>> + * synchronization see implementation details of Intel
>> + * LBR call stack data profiling;
>> + */
>> + if (ctx->task_ctx_data && next_ctx->task_ctx_data)
>> + pmu->sync_task_ctx(next_ctx->task_ctx_data,
>> + ctx->task_ctx_data);
>
> This still does not check if pmu->sync_task_ctx is set. If any other
> arch ever uses task_ctx_data without then also supplying this method
> things will go *bang*.
>
> Also, I think I prefer the variant I gave you yesterday:
>
> https://lkml.kernel.org/r/[email protected]
>
> if (pmu->swap_task_ctx)
> pmu->swap_task_ctx(ctx, next_ctx);
> else
> swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
>
> That also unconfuses the argument order in your above patch (where you
> have to undo thw swap).
>
> Alternatively, since there currently is no other arch using
> task_ctx_data, we can make the pmu::swap_task_ctx() thing mandatory when
> having it and completely replace the swap(), write it like so:
>
>
> - swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
It still has to be swapped unconditionally. Thus, it will be a part of
architecture specific implementation:
void intel_pmu_lbr_sync_task_ctx(struct x86_perf_task_context **prev,
struct x86_perf_task_context **next)
{
if (*prev && *next) {
swap(*prev->lbr_callstack_users, *next->lbr_callstack_users);
...
}
swap(prev, next);
}
> + if (pmu->swap_task_ctx)
> + pmu->swap_task_ctx(ctx, next_ctx);
>
> Hmm?
This option above looks attractive because it pushes complexity down
towards architecture specific implementation.
However, in order to keep the existing performance at the same level
if (ctx->task_ctx_data && next_ctx->task_ctx_data) check has to be
preserved as closer to the top layer as possible. So the fastest version
appears to look like this:
swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
if (ctx->task_ctx_data && next_ctx->task_ctx_data && pmu->sync_task_ctx)
pmu->sync_task_ctx(next_ctx->task_ctx_data, ctx->task_ctx_data);
If some architecture needs specific synchronization then it is enough
to implement sync_task_ctx() without changing the core.
~Alexey