From: Kan Liang <[email protected]>
Reading LBR registers in a perf NMI handler for a non-PEBS event
causes a high overhead because the number of LBR registers is huge.
To reduce the overhead, the XSAVES instruction should be used to replace
the LBR registers' reading method.
The XSAVES buffer used for LBR read has to be per-CPU because the NMI
handler invoked the lbr_read(). The existing task_ctx_data buffer
cannot be used which is per-task and only be allocated for the LBR call
stack mode. A new lbr_xsave pointer is introduced in the cpu_hw_events
as an XSAVES buffer for LBR read.
The XSAVES buffer should be allocated only when LBR is used by a
non-PEBS event on the CPU because the total size of the lbr_xsave is
not small (~1.4KB).
The XSAVES buffer is allocated when a non-PEBS event is added, but it
is lazily released in x86_release_hardware() when perf releases the
entire PMU hardware resource, because perf may frequently schedule the
event, e.g. high context switch. The lazy release method reduces the
overhead of frequently allocate/free the buffer.
If the lbr_xsave fails to be allocated, roll back to normal Arch LBR
lbr_read().
Reviewed-by: Dave Hansen <[email protected]>
Signed-off-by: Kan Liang <[email protected]>
---
arch/x86/events/core.c | 1 +
arch/x86/events/intel/lbr.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
arch/x86/events/perf_event.h | 7 ++++++
3 files changed, 65 insertions(+), 1 deletion(-)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index aeb6e6d..3339347 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -359,6 +359,7 @@ void x86_release_hardware(void)
if (atomic_dec_and_mutex_lock(&pmc_refcount, &pmc_reserve_mutex)) {
release_pmc_hardware();
release_ds_buffers();
+ release_lbr_buffers();
mutex_unlock(&pmc_reserve_mutex);
}
}
diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index dc40a76..4b0042f 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -624,6 +624,7 @@ static inline bool branch_user_callstack(unsigned br_sel)
void intel_pmu_lbr_add(struct perf_event *event)
{
+ struct kmem_cache *kmem_cache = event->pmu->task_ctx_cache;
struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
if (!x86_pmu.lbr_nr)
@@ -658,6 +659,28 @@ void intel_pmu_lbr_add(struct perf_event *event)
perf_sched_cb_inc(event->ctx->pmu);
if (!cpuc->lbr_users++ && !event->total_time_running)
intel_pmu_lbr_reset();
+
+ if (x86_pmu.arch_lbr && kmem_cache && !cpuc->lbr_xsave &&
+ (cpuc->lbr_users != cpuc->lbr_pebs_users))
+ cpuc->lbr_xsave = kmem_cache_alloc(kmem_cache, GFP_KERNEL);
+}
+
+void release_lbr_buffers(void)
+{
+ struct kmem_cache *kmem_cache = x86_get_pmu()->task_ctx_cache;
+ struct cpu_hw_events *cpuc;
+ int cpu;
+
+ if (!x86_pmu.arch_lbr)
+ return;
+
+ for_each_possible_cpu(cpu) {
+ cpuc = per_cpu_ptr(&cpu_hw_events, cpu);
+ if (kmem_cache && cpuc->lbr_xsave) {
+ kmem_cache_free(kmem_cache, cpuc->lbr_xsave);
+ cpuc->lbr_xsave = NULL;
+ }
+ }
}
void intel_pmu_lbr_del(struct perf_event *event)
@@ -909,6 +932,38 @@ static void intel_pmu_arch_lbr_read(struct cpu_hw_events *cpuc)
cpuc->lbr_stack.nr = i;
}
+static void intel_pmu_arch_lbr_read_xsave(struct cpu_hw_events *cpuc)
+{
+ struct x86_perf_task_context_arch_lbr_xsave *xsave = cpuc->lbr_xsave;
+ struct arch_lbr_entry *lbr;
+ int i;
+
+ if (!xsave)
+ goto rollback;
+
+ copy_dynamic_supervisor_to_kernel(&xsave->xsave, XFEATURE_MASK_LBR);
+
+ for (i = 0; i < x86_pmu.lbr_nr; i++) {
+ lbr = &xsave->lbr.entries[i];
+
+ /*
+ * Read LBR entries until invalid entry (0s) is detected.
+ */
+ if (!lbr->lbr_from)
+ break;
+
+ __intel_pmu_arch_lbr_read(cpuc, i, lbr->lbr_from,
+ lbr->lbr_to, lbr->lbr_info);
+ }
+
+ cpuc->lbr_stack.nr = i;
+
+ return;
+
+rollback:
+ intel_pmu_arch_lbr_read(cpuc);
+}
+
void intel_pmu_lbr_read(void)
{
struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
@@ -1757,14 +1812,15 @@ void __init intel_pmu_arch_lbr_init(void)
x86_pmu.lbr_enable = intel_pmu_arch_lbr_enable;
x86_pmu.lbr_disable = intel_pmu_arch_lbr_disable;
x86_pmu.lbr_reset = intel_pmu_arch_lbr_reset;
- x86_pmu.lbr_read = intel_pmu_arch_lbr_read;
if (arch_lbr_xsave) {
x86_pmu.lbr_save = intel_pmu_arch_lbr_xsaves;
x86_pmu.lbr_restore = intel_pmu_arch_lbr_xrstors;
+ x86_pmu.lbr_read = intel_pmu_arch_lbr_read_xsave;
pr_cont("XSAVE ");
} else {
x86_pmu.lbr_save = intel_pmu_arch_lbr_save;
x86_pmu.lbr_restore = intel_pmu_arch_lbr_restore;
+ x86_pmu.lbr_read = intel_pmu_arch_lbr_read;
}
x86_pmu.arch_lbr = true;
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 812980e..b9dfc55 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -251,6 +251,7 @@ struct cpu_hw_events {
u64 br_sel;
void *last_task_ctx;
int last_log_id;
+ void *lbr_xsave;
/*
* Intel host/guest exclude bits
@@ -1106,6 +1107,8 @@ void release_ds_buffers(void);
void reserve_ds_buffers(void);
+void release_lbr_buffers(void);
+
extern struct event_constraint bts_constraint;
void intel_pmu_enable_bts(u64 config);
@@ -1250,6 +1253,10 @@ static inline void release_ds_buffers(void)
{
}
+static inline void release_lbr_buffers(void)
+{
+}
+
static inline int intel_pmu_init(void)
{
return 0;
--
2.7.4
On Fri, Jun 19, 2020 at 07:04:09AM -0700, [email protected] wrote:
...
> +static void intel_pmu_arch_lbr_read_xsave(struct cpu_hw_events *cpuc)
> +{
> + struct x86_perf_task_context_arch_lbr_xsave *xsave = cpuc->lbr_xsave;
> + struct arch_lbr_entry *lbr;
> + int i;
> +
> + if (!xsave)
> + goto rollback;
Why not make it simplier?
if (!xsave) {
intel_pmu_arch_lbr_read(cpuc);
return;
}
The goto and "return" statement before the "rollback" label
looks pretty ugly. I'm sorry I didn't follow the series
in details so if you plan to add more handlers at "rollback"
then sure.
On 6/22/2020 2:49 PM, Cyrill Gorcunov wrote:
> On Fri, Jun 19, 2020 at 07:04:09AM -0700, [email protected] wrote:
> ...
>> +static void intel_pmu_arch_lbr_read_xsave(struct cpu_hw_events *cpuc)
>> +{
>> + struct x86_perf_task_context_arch_lbr_xsave *xsave = cpuc->lbr_xsave;
>> + struct arch_lbr_entry *lbr;
>> + int i;
>> +
>> + if (!xsave)
>> + goto rollback;
>
> Why not make it simplier?
>
> if (!xsave) {
> intel_pmu_arch_lbr_read(cpuc);
> return;
> }
>
> The goto and "return" statement before the "rollback" label
> looks pretty ugly. I'm sorry I didn't follow the series
> in details so if you plan to add more handlers at "rollback"
> then sure.
>
There were several handlers when I first implemented the function, but
they are removed now. I don't think I will add more handlers in the next
version.
I will remove the "rollback" label.
Thanks for pointing it out.
Kan
On Mon, Jun 22, 2020 at 03:11:07PM -0400, Liang, Kan wrote:
> >
> > The goto and "return" statement before the "rollback" label
> > looks pretty ugly. I'm sorry I didn't follow the series
> > in details so if you plan to add more handlers at "rollback"
> > then sure.
> >
>
> There were several handlers when I first implemented the function, but they
> are removed now. I don't think I will add more handlers in the next version.
> I will remove the "rollback" label.
>
> Thanks for pointing it out.
This could be done on top of the series of course, no need to resend
for this sole change I think.