perf/x86: implement HT counter corruption workaround

From: Maria Dimakopoulou <[email protected]>

This patch adds a new shared_regs style structure to the per-cpu x86 state (cpuc). It is used to coordinate access between counters which must be used with exclusion across HyperThreads on Intel processors. This new struct is not needed on each PMU, thus is is allocated on demand.

Reviewed-by Stephane Eranian <[email protected]>

Signed-off-by Maria Dimakopoulou <[email protected]>

---
 arch/x86/kernel/cpu/perf_event.h       |   40 ++++++++++++++++++--
 arch/x86/kernel/cpu/perf_event_intel.c |   63 +++++++++++++++++++++++++++++---
 2 files changed, 94 insertions(+), 9 deletions(-)

arch/x86/kernel/cpu/perf_event.h +15 -4

index 413799f..5da0a2b 100644
@@ -65,10 +65,11 @@ struct event_constraint {
 /*
  * struct hw_perf_event.flags flags
  */
-#define PERF_X86_EVENT_PEBS_LDLAT	0x1 /* ld+ldlat data address sampling */
-#define PERF_X86_EVENT_PEBS_ST		0x2 /* st data address sampling */
-#define PERF_X86_EVENT_PEBS_ST_HSW	0x4 /* haswell style st data sampling */
-#define PERF_X86_EVENT_COMMITTED	0x8 /* event passed commit_txn */
+#define PERF_X86_EVENT_PEBS_LDLAT	0x01 /* ld+ldlat data address sampling */
+#define PERF_X86_EVENT_PEBS_ST		0x02 /* st data address sampling */
+#define PERF_X86_EVENT_PEBS_ST_HSW	0x04 /* haswell style st data sampling */
+#define PERF_X86_EVENT_COMMITTED	0x08 /* event passed commit_txn */
+#define PERF_X86_EVENT_EXCL		0x10 /* HT exclusivity on counter */
 
 struct amd_nb {
 	int nb_id;  /* NorthBridge id */

@@ -119,6 +120,27 @@ struct intel_shared_regs {
 	unsigned                core_id;	/* per-core: core id */
 };
 
+enum intel_excl_state_type {
+	INTEL_EXCL_UNUSED    = 0, /* counter is unused */
+	INTEL_EXCL_SHARED    = 1, /* counter can be used by both threads */
+	INTEL_EXCL_EXCLUSIVE = 2, /* counter can be used by one thread only */
+};
+
+struct intel_excl_states {
+	enum intel_excl_state_type init_state[X86_PMC_IDX_MAX];
+	enum intel_excl_state_type state[X86_PMC_IDX_MAX];
+};

Quoted 55 lines · Stephane Eranian

From: Maria Dimakopoulou <[email protected]>

This patch adds a new shared_regs style structure to the per-cpu x86 state (cpuc). It is used to coordinate access between counters which must be used with exclusion across HyperThreads on Intel processors. This new struct is not needed on each PMU, thus is is allocated on demand.

Reviewed-by Stephane Eranian <[email protected]>

Signed-off-by Maria Dimakopoulou <[email protected]>

---
 arch/x86/kernel/cpu/perf_event.h       |   40 ++++++++++++++++++--
 arch/x86/kernel/cpu/perf_event_intel.c |   63 +++++++++++++++++++++++++++++---
 2 files changed, 94 insertions(+), 9 deletions(-)

arch/x86/kernel/cpu/perf_event.h +21 -4

index 413799f..5da0a2b 100644
@@ -65,10 +65,11 @@ struct event_constraint {
 /*
  * struct hw_perf_event.flags flags
  */
-#define PERF_X86_EVENT_PEBS_LDLAT    0x1 /* ld+ldlat data address sampling */
-#define PERF_X86_EVENT_PEBS_ST               0x2 /* st data address sampling */
-#define PERF_X86_EVENT_PEBS_ST_HSW   0x4 /* haswell style st data sampling */
-#define PERF_X86_EVENT_COMMITTED     0x8 /* event passed commit_txn */
+#define PERF_X86_EVENT_PEBS_LDLAT    0x01 /* ld+ldlat data address sampling */
+#define PERF_X86_EVENT_PEBS_ST               0x02 /* st data address sampling */
+#define PERF_X86_EVENT_PEBS_ST_HSW   0x04 /* haswell style st data sampling */
+#define PERF_X86_EVENT_COMMITTED     0x08 /* event passed commit_txn */
+#define PERF_X86_EVENT_EXCL          0x10 /* HT exclusivity on counter */
 
 struct amd_nb {
      int nb_id;  /* NorthBridge id */

@@ -119,6 +120,27 @@ struct intel_shared_regs {
      unsigned                core_id;        /* per-core: core id */
 };
 
+enum intel_excl_state_type {
+     INTEL_EXCL_UNUSED    = 0, /* counter is unused */
+     INTEL_EXCL_SHARED    = 1, /* counter can be used by both threads */
+     INTEL_EXCL_EXCLUSIVE = 2, /* counter can be used by one thread only */
+};
+
+struct intel_excl_states {
+     enum intel_excl_state_type init_state[X86_PMC_IDX_MAX];
+     enum intel_excl_state_type state[X86_PMC_IDX_MAX];
+};
+
+struct intel_excl_cntrs {
+     spinlock_t      lock;
+     unsigned long   lock_flags;
+
+     struct intel_excl_states states[2];

arch/x86/kernel/cpu/perf_event.h struct intel_shared_regs { · line 142

Quoted 4 lines

arch/x86/kernel/cpu/perf_event.h +4 -0

+
+     int             refcnt;         /* per-core: #HT threads */
+     unsigned        core_id;        /* per-core: core id */
+};

This must be a raw_spin_lock_t, its taken from pmu::add() which is called under perf_event_context::lock, which is raw_spinlock_t, as its taken under rq::lock, which too is raw_spinlock_t.

I should really get around to fixing these errors and include the lockdep infrastructure for this.

arch/x86/kernel/cpu/perf_event.h do { \ · line 533

Quoted 5 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event.h +1 -0

@@ -499,6 +530,7 @@ do {                                                                      \
  */
 #define PMU_FL_NO_HT_SHARING 0x1 /* no hyper-threading resource sharing */
 #define PMU_FL_HAS_RSP_1     0x2 /* has 2 equivalent offcore_rsp regs   */
+#define PMU_FL_EXCL_CNTRS    0x4 /* has exclusive counter requirements  */

the EXLC thing is HT specific too, right? How about HT_EXCL or so?

arch/x86/kernel/cpu/perf_event_intel.c +7 -0

@@ -2020,12 +2050,29 @@ static void intel_pmu_cpu_starting(int cpu)
 
 	if (x86_pmu.lbr_sel_map)
 		cpuc->lbr_sel = &cpuc->shared_regs->regs[EXTRA_REG_LBR];
+
+	if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
+		for_each_cpu(i, topology_thread_cpumask(cpu)) {
+			struct intel_excl_cntrs *c;
+
+			c = per_cpu(cpu_hw_events, i).excl_cntrs;
+			if (c && c->core_id == core_id) {

This hard assumes theres only ever 2 threads, which is true and I suppose more in arch/x86 will come apart the moment Intel makes a chip with more, still, do we have topology_thread_id() or so to cure this?

Quoted 3 lines

This hard assumes theres only ever 2 threads, which is true and I suppose more in arch/x86 will come apart the moment Intel makes a chip with more, still, do we have topology_thread_id() or so to cure this?

Xeon Phi already has 4 threads today.

-Andi

--
[email protected] -- Speaking for myself only

Quoted 17 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +13 -0

@@ -2020,12 +2050,29 @@ static void intel_pmu_cpu_starting(int cpu)
 
      if (x86_pmu.lbr_sel_map)
              cpuc->lbr_sel = &cpuc->shared_regs->regs[EXTRA_REG_LBR];
+
+     if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
+             for_each_cpu(i, topology_thread_cpumask(cpu)) {
+                     struct intel_excl_cntrs *c;
+
+                     c = per_cpu(cpu_hw_events, i).excl_cntrs;
+                     if (c && c->core_id == core_id) {
+                             cpuc->kfree_on_online[1] = cpuc->excl_cntrs;
+                             cpuc->excl_cntrs = c;
+                             cpuc->excl_thread_id = 1;
+                             break;
+                     }
+             }

arch/x86/kernel/cpu/perf_event_intel.c static void intel_pmu_cpu_starting(int cpu) · line 2069

Quoted 4 lines

arch/x86/kernel/cpu/perf_event_intel.c +3 -0

+             cpuc->excl_cntrs->core_id = core_id;
+             cpuc->excl_cntrs->refcnt++;
+     }
 }

This hard assumes theres only ever 2 threads, which is true and I suppose more in arch/x86 will come apart the moment Intel makes a chip with more, still, do we have topology_thread_id() or so to cure this?

Quoted 18 lines · Peter Zijlstra

Quoted 17 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +13 -0

@@ -2020,12 +2050,29 @@ static void intel_pmu_cpu_starting(int cpu)
 
      if (x86_pmu.lbr_sel_map)
              cpuc->lbr_sel = &cpuc->shared_regs->regs[EXTRA_REG_LBR];
+
+     if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
+             for_each_cpu(i, topology_thread_cpumask(cpu)) {
+                     struct intel_excl_cntrs *c;
+
+                     c = per_cpu(cpu_hw_events, i).excl_cntrs;
+                     if (c && c->core_id == core_id) {
+                             cpuc->kfree_on_online[1] = cpuc->excl_cntrs;
+                             cpuc->excl_cntrs = c;
+                             cpuc->excl_thread_id = 1;
+                             break;
+                     }
+             }

Quoted 8 lines

arch/x86/kernel/cpu/perf_event_intel.c static void intel_pmu_cpu_starting(int cpu) · line 2069

Quoted 4 lines

arch/x86/kernel/cpu/perf_event_intel.c +3 -0

+             cpuc->excl_cntrs->core_id = core_id;
+             cpuc->excl_cntrs->refcnt++;
+     }
 }

This hard assumes theres only ever 2 threads, which is true and I suppose more in arch/x86 will come apart the moment Intel makes a chip with more, still, do we have topology_thread_id() or so to cure this?

I assume your comment is relative to kfree_on_online[].
This code is specific to the HT bug, so yes, it assumes 2 threads and that only one entry of the two excl_cntrs structs needs to be freed. Doing otherwise, would require a list and will never be used to its full potential.

static void

arch/x86/kernel/cpu/perf_event_intel.c +19 -0

+intel_start_scheduling(struct cpu_hw_events *cpuc)
+{
 
+	/*
+	 * lock shared state until we are done scheduling
+	 * in stop_event_scheduling()
+	 * makes scheduling appear as a transaction
+	 */
+	spin_lock_irqsave(&excl_cntrs->lock, excl_cntrs->lock_flags);
+
+	/*
+	 * save initial state of sibling thread
+	 */
+	memcpy(xlo->init_state, xlo->state, sizeof(xlo->init_state));
+}
+
+static void
+intel_stop_scheduling(struct cpu_hw_events *cpuc)
+{
 
+	/*

arch/x86/kernel/cpu/perf_event_intel.c +17 -0

+static void intel_commit_scheduling(struct cpu_hw_events *cpuc,
+				    struct perf_event *event, int cntr)
+{
+	struct intel_excl_cntrs *excl_cntrs = cpuc->excl_cntrs;
+	struct event_constraint *c = event->hw.constraint;
+	struct intel_excl_states *xlo, *xl;
+	int tid = cpuc->excl_thread_id;
+	int o_tid = 1 - tid;
+	int is_excl;
+
+	if (cpuc->is_fake || !c)
+		return;
+
+	is_excl = c->flags & PERF_X86_EVENT_EXCL;
+
+	if (!(c->flags & PERF_X86_EVENT_DYNAMIC))
+		return;

arch/x86/kernel/cpu/perf_event_intel.c +7 -0

+
+	if (cntr >= 0) {
+		if (is_excl)
+			xlo->init_state[cntr] = INTEL_EXCL_EXCLUSIVE;
+		else
+			xlo->init_state[cntr] = INTEL_EXCL_SHARED;
+	}
 }

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

+static struct event_constraint *
+intel_get_excl_constraints(struct cpu_hw_events *cpuc, struct perf_event *event,
+			   struct event_constraint *c)
+{
 

Quoted 5 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

+static struct event_constraint *
+intel_get_excl_constraints(struct cpu_hw_events *cpuc, struct perf_event *event,
+                        struct event_constraint *c)
+{
 

arch/x86/kernel/cpu/perf_event_intel.c intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event · line 1776

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +10 -0

+     if (!(c->flags & PERF_X86_EVENT_DYNAMIC)) {
+
+             /*
+              * in case we fail, we assume no counter
+              * is supported to be on the safe side
+              */
+             cx = kmalloc(sizeof(*cx), GFP_KERNEL);
+             if (!cx)
+                     return &emptyconstraint;
+

Ok, so forgive me if I'm wrong, but the way we get here is through:

x86_schedule_event()
->start_scheduling()

    spin_lock()

->get_event_constraints()
intel_get_excl_constraints()
kmalloc(.gfp=GFP_KERNEL)

How can that ever work?

Are you saying it is illegal to call kmalloc() from this context?

kmalloc is needed because we need to allocate a new constraint struct since the static constraint cannot be modified.

Worst case we can statically allocate a second
constraint struct in the event struct.

Quoted 27 lines · Peter Zijlstra

Quoted 5 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

+static struct event_constraint *
+intel_get_excl_constraints(struct cpu_hw_events *cpuc, struct perf_event *event,
+                        struct event_constraint *c)
+{
 

arch/x86/kernel/cpu/perf_event_intel.c intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event · line 1776

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +10 -0

+     if (!(c->flags & PERF_X86_EVENT_DYNAMIC)) {
+
+             /*
+              * in case we fail, we assume no counter
+              * is supported to be on the safe side
+              */
+             cx = kmalloc(sizeof(*cx), GFP_KERNEL);
+             if (!cx)
+                     return &emptyconstraint;
+

Ok, so forgive me if I'm wrong, but the way we get here is through:

x86_schedule_event()
->start_scheduling()

    spin_lock()

->get_event_constraints()
intel_get_excl_constraints()
kmalloc(.gfp=GFP_KERNEL)

Quoted 1 line

How can that ever work?

Are you saying it is illegal to call kmalloc() from this context?

kmalloc is needed because we need to allocate a new constraint struct since the static constraint cannot be modified.

Worst case we can statically allocate a second
constraint struct in the event struct.

Quoted 7 lines

Quoted 7 lines

x86_schedule_event()
->start_scheduling()

    spin_lock()

->get_event_constraints()
intel_get_excl_constraints()
kmalloc(.gfp=GFP_KERNEL)

Quoted 4 lines

Quoted 1 line

How can that ever work?

Are you saying it is illegal to call kmalloc() from this context?

Nobody will come and arrest you for it, so no. Broken though. GFP_KERNEL will attempt to sleep to wait for reclaim, and you're holding a spinlock.

Quoted 7 lines · Peter Zijlstra

Quoted 7 lines

Quoted 7 lines

x86_schedule_event()
->start_scheduling()

    spin_lock()

->get_event_constraints()
intel_get_excl_constraints()
kmalloc(.gfp=GFP_KERNEL)

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

How can that ever work?

Are you saying it is illegal to call kmalloc() from this context?

Nobody will come and arrest you for it, so no. Broken though. GFP_KERNEL will attempt to sleep to wait for reclaim, and you're holding a spinlock.

Furthermore, even GFP_ATOMIC shouldn't be used because these are/should be raw_spinlocks and the zone->lock is a regular spinlock.

So please look into the preallocation thing.

arch/x86/kernel/cpu/perf_event_intel.c +20 -0

+
+	/*
+	 * Modify static constraint with current dynamic
+	 * state of thread
+	 *
+	 * EXCLUSIVE: sibling counter measuring exclusive event
+	 * SHARED   : sibling counter measuring non-exclusive event
+	 * UNUSED   : sibling counter unused
+	 */
+	for_each_set_bit(i, cx->idxmsk, X86_PMC_IDX_MAX) {
+		/*
+		 * exclusive event in sibling counter
+		 * our corresponding counter cannot be used
+		 * regardless of our event
+		 */
+		if (xl->state[i] == INTEL_EXCL_EXCLUSIVE)
+			__clear_bit(i, cx->idxmsk);
+		/*
+		 * if measuring an exclusive event, sibling
+		 * measuring non-exclusive, then counter cannot

Quoted 20 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +20 -0

+
+     /*
+      * Modify static constraint with current dynamic
+      * state of thread
+      *
+      * EXCLUSIVE: sibling counter measuring exclusive event
+      * SHARED   : sibling counter measuring non-exclusive event
+      * UNUSED   : sibling counter unused
+      */
+     for_each_set_bit(i, cx->idxmsk, X86_PMC_IDX_MAX) {
+             /*
+              * exclusive event in sibling counter
+              * our corresponding counter cannot be used
+              * regardless of our event
+              */
+             if (xl->state[i] == INTEL_EXCL_EXCLUSIVE)
+                     __clear_bit(i, cx->idxmsk);
+             /*
+              * if measuring an exclusive event, sibling
+              * measuring non-exclusive, then counter cannot

arch/x86/kernel/cpu/perf_event_intel.c intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event · line 1825

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +10 -0

+              * be used
+              */
+             if (is_excl && xl->state[i] == INTEL_EXCL_SHARED)
+                     __clear_bit(i, cx->idxmsk);
+     }
+
+     /*
+      * recompute actual bit weight for scheduling algorithm
+      */
+     cx->weight = hweight64(cx->idxmsk64);

So I think we talked about this a bit; what happens if CPU0 (taking your 4 core HSW-client) is first to program its counters and takes all 4 in exclusive mode?

Then there's none left for CPU4.

Did I miss where we avoid that problem, or is that an actual issue?

Quoted 36 lines · Peter Zijlstra

Quoted 20 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +20 -0

+
+     /*
+      * Modify static constraint with current dynamic
+      * state of thread
+      *
+      * EXCLUSIVE: sibling counter measuring exclusive event
+      * SHARED   : sibling counter measuring non-exclusive event
+      * UNUSED   : sibling counter unused
+      */
+     for_each_set_bit(i, cx->idxmsk, X86_PMC_IDX_MAX) {
+             /*
+              * exclusive event in sibling counter
+              * our corresponding counter cannot be used
+              * regardless of our event
+              */
+             if (xl->state[i] == INTEL_EXCL_EXCLUSIVE)
+                     __clear_bit(i, cx->idxmsk);
+             /*
+              * if measuring an exclusive event, sibling
+              * measuring non-exclusive, then counter cannot

arch/x86/kernel/cpu/perf_event_intel.c intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event · line 1825

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +10 -0

+              * be used
+              */
+             if (is_excl && xl->state[i] == INTEL_EXCL_SHARED)
+                     __clear_bit(i, cx->idxmsk);
+     }
+
+     /*
+      * recompute actual bit weight for scheduling algorithm
+      */
+     cx->weight = hweight64(cx->idxmsk64);

So I think we talked about this a bit; what happens if CPU0 (taking your 4 core HSW-client) is first to program its counters and takes all 4 in exclusive mode?

Quoted 3 lines

Then there's none left for CPU4.

Did I miss where we avoid that problem, or is that an actual issue?

Yes, this patch series does not address this problem yet. It will be in a second series.
Don't have a good solution yet.

Quoted 41 lines · Stephane Eranian

Quoted 39 lines · Peter Zijlstra

Quoted 20 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +20 -0

+
+     /*
+      * Modify static constraint with current dynamic
+      * state of thread
+      *
+      * EXCLUSIVE: sibling counter measuring exclusive event
+      * SHARED   : sibling counter measuring non-exclusive event
+      * UNUSED   : sibling counter unused
+      */
+     for_each_set_bit(i, cx->idxmsk, X86_PMC_IDX_MAX) {
+             /*
+              * exclusive event in sibling counter
+              * our corresponding counter cannot be used
+              * regardless of our event
+              */
+             if (xl->state[i] == INTEL_EXCL_EXCLUSIVE)
+                     __clear_bit(i, cx->idxmsk);
+             /*
+              * if measuring an exclusive event, sibling
+              * measuring non-exclusive, then counter cannot

arch/x86/kernel/cpu/perf_event_intel.c intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event · line 1825

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +10 -0

+              * be used
+              */
+             if (is_excl && xl->state[i] == INTEL_EXCL_SHARED)
+                     __clear_bit(i, cx->idxmsk);
+     }
+
+     /*
+      * recompute actual bit weight for scheduling algorithm
+      */
+     cx->weight = hweight64(cx->idxmsk64);

So I think we talked about this a bit; what happens if CPU0 (taking your 4 core HSW-client) is first to program its counters and takes all 4 in exclusive mode?

Then there's none left for CPU4.

Did I miss where we avoid that problem, or is that an actual issue?

Quoted 3 lines

Yes, this patch series does not address this problem yet. It will be in a second series.
Don't have a good solution yet.

We could limit each cpu to num_counters/2 exclusive slots. That'll still be painful with some constrained events I imagine, but in general that should 'work' I suppose.

Quoted 3 lines · Peter Zijlstra

We could limit each cpu to num_counters/2 exclusive slots. That'll still be painful with some constrained events I imagine, but in general that should 'work' I suppose.

That is probably the easiest solution, just modify the dynamic constraint mask some more. Have not yet tried it.

The repatriation of the leaked count is not so easy either. Need to IPI the other HT. There may be some restrictions as to when we can safely do this.

Quoted 41 lines · Stephane Eranian

Quoted 39 lines · Peter Zijlstra

Quoted 20 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +20 -0

+
+     /*
+      * Modify static constraint with current dynamic
+      * state of thread
+      *
+      * EXCLUSIVE: sibling counter measuring exclusive event
+      * SHARED   : sibling counter measuring non-exclusive event
+      * UNUSED   : sibling counter unused
+      */
+     for_each_set_bit(i, cx->idxmsk, X86_PMC_IDX_MAX) {
+             /*
+              * exclusive event in sibling counter
+              * our corresponding counter cannot be used
+              * regardless of our event
+              */
+             if (xl->state[i] == INTEL_EXCL_EXCLUSIVE)
+                     __clear_bit(i, cx->idxmsk);
+             /*
+              * if measuring an exclusive event, sibling
+              * measuring non-exclusive, then counter cannot

arch/x86/kernel/cpu/perf_event_intel.c intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event · line 1825

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +10 -0

+              * be used
+              */
+             if (is_excl && xl->state[i] == INTEL_EXCL_SHARED)
+                     __clear_bit(i, cx->idxmsk);
+     }
+
+     /*
+      * recompute actual bit weight for scheduling algorithm
+      */
+     cx->weight = hweight64(cx->idxmsk64);

So I think we talked about this a bit; what happens if CPU0 (taking your 4 core HSW-client) is first to program its counters and takes all 4 in exclusive mode?

Then there's none left for CPU4.

Did I miss where we avoid that problem, or is that an actual issue?

Quoted 3 lines

Yes, this patch series does not address this problem yet. It will be in a second series.
Don't have a good solution yet.

We could limit each cpu to num_counters/2 exclusive slots. That'll still be painful with some constrained events I imagine, but in general that should 'work' I suppose.

From: Maria Dimakopoulou <[email protected]>

This patch adds a sysfs entry:

/sys/devices/cpu/ht_bug_workaround

to activate/deactivate the PMU HT bug workaround.

To activate (activated by default):

 # echo 1 > /sys/devices/cpu/ht_bug_workaround

To deactivate:

 # echo 0 > /sys/devices/cpu/ht_bug_workaround

Quoted 6 lines · Stephane Eranian

From: Maria Dimakopoulou <[email protected]>

This patch adds a sysfs entry:

/sys/devices/cpu/ht_bug_workaround

Quoted 7 lines

to activate/deactivate the PMU HT bug workaround.

To activate (activated by default):

 # echo 1 > /sys/devices/cpu/ht_bug_workaround

To deactivate:

 # echo 0 > /sys/devices/cpu/ht_bug_workaround

If your hardware contains this erratum, why would you ever want to disable the workaround? Providing the user with the option of turning this off seems like a bad idea.

I suspect that users will rarely know whether they can legitimately disable this.

If you know what you are doing (poweruser), then there are measurements which works fine with the HT erratum. This is why we have the option.

For instance if you only measure events 4x4 in system-wide mode and you know which counters these event are going to use, you don't need the workaround. For instance:

# perf stat -a -e r81d0,r01d1,r08d0,r20d1 sleep 5

Works well if you have a uniform workload across all CPUs. All those events leak, but the leaks balance themselves and you get the correct counts in the end. The advantage is that you don't have to multiplex. With the workaround enable, this would multiplex a lot.

But as I said, this is for experts only.

Another reason is for systems with HT disabled. It turned out to be very difficult to determine at kernel BOOT TIME if HT was enabled or not. Note what I said: ENABLED and not SUPPORTED. The latter is easy to detect. The former needs some model specific code which is quite complicated. I wish the kernel had this capability abstracted somehow. Consequently, the workaround is always enabled. When HT is disabled, there won't be multiplexing because there will never be conflict, but you pay a little price for accessing the extra data state.

An init script could well detect HT is off and thus disable the workaround altogether.

Those are the two main reasons for this control in sysfs.

Quoted 16 lines · Stephane Eranian

If you know what you are doing (poweruser), then there are measurements which works fine with the HT erratum. This is why we have the option.

For instance if you only measure events 4x4 in system-wide mode and you know which counters these event are going to use, you don't need the workaround. For instance:

# perf stat -a -e r81d0,r01d1,r08d0,r20d1 sleep 5

Works well if you have a uniform workload across all CPUs. All those events leak, but the leaks balance themselves and you get the correct counts in the end. The advantage is that you don't have to multiplex. With the workaround enable, this would multiplex a lot.

Quoted 1 line

But as I said, this is for experts only.

Is it not possible to detect this in the kernel and only enable the workaround for the case where the leaks don't balance? It may not be possible (or practical) but I do think it's worth having the discussion.

Quoted 9 lines

Another reason is for systems with HT disabled. It turned out to be very difficult to determine at kernel BOOT TIME if HT was enabled or not. Note what I said: ENABLED and not SUPPORTED. The latter is easy to detect. The former needs some model specific code which is quite complicated. I wish the kernel had this capability abstracted somehow. Consequently, the workaround is always enabled. When HT is disabled, there won't be multiplexing because there will never be conflict, but you pay a little price for accessing the extra data state.

Does cpu_sibling_map not give you some indication of whether HT is enabled? I think the topology_thread_cpumask() is the topology API for that. But I could most definitely be wrong. Hopefully someone on the Cc list will know.

Quoted 1 line

An init script could well detect HT is off and thus disable the workaround altogether.

This is exactly the kind of thing I think we should try to avoid. The ideal is that things just work out of the box and don't require these magic knobs to be tweaked.

Quoted 1 line

Those are the two main reasons for this control in sysfs.

Thanks for the info!

How would you know that you have a uniform workload from inside the kernel?

Quoted 5 lines

Does cpu_sibling_map not give you some indication of whether HT is enabled? I think the topology_thread_cpumask() is the topology API for that. But I could most definitely be wrong. Hopefully someone on the Cc list will know.

Remember trying some of that, but when perf_event is initialized, those masks are not yet setup properly.

Quoted 2 lines · Stephane Eranian

How would you know that you have a uniform workload from inside the kernel?

That's what I'm asking you ;-)

Quoted 7 lines

Quoted 5 lines

Does cpu_sibling_map not give you some indication of whether HT is enabled? I think the topology_thread_cpumask() is the topology API for that. But I could most definitely be wrong. Hopefully someone on the Cc list will know.

Remember trying some of that, but when perf_event is initialized, those masks are not yet setup properly.

Oh, bummer.

If there's no way to detect whether we should enable this workaround at runtime (and it sounds like there isn't a good way), then that's fair enough.

We should think twice about allowing it to be disabled via sysfs, however. Because what is guaranteed to happen is that some user will report getting bogus results from perf for these events and we'll spend days trying to figure out why, only to discover they disabled the workaround and didn't tell us or didn't realise that they'd disabled it.

If the workaround is low overhead, can't we just leave it enabled?

It is enabled by default. Nothing is done to try and disable it later even once the kernel is fully booted. So this is mostly for testing and power-users.

Quoted 3 lines · Stephane Eranian

It is enabled by default. Nothing is done to try and disable it later even once the kernel is fully booted. So this is mostly for testing and power-users.

You keep saying "power-users". What is the disadvantage for power users running with the workaround disabled? I.e., why would anyone want to disable it at all, what is the use case for that?

I gave a test case earlier:

# echo 0 >/proc/sys/kernel/nmi_watchdog
# run_my_uniform_workload_on_all_cpus &
# perf stat -a -e r81d0,r01d1,r08d0,r20d1 sleep 5

That run gives the correct answer.

If I just look at CPU0 CPU4 siblings:

CPU0, counter0 leaks N counts to CPU4, counter 0

but at the same time:

CPU4, counter0 leaks N counts to CPU0, counter 0

This is because we have the same event in the same counter AND the workload is uniform, meaning the event (here loads retired) occurs at the same rate on both siblings.

You can test this by measuring only on one HT.

# perf stat -a -C0 -e r81d0,r01d1,r08d0,r20d1 sleep 5

Note that some events, leak more than they count.

Quoted 24 lines · Stephane Eranian

I gave a test case earlier:

# echo 0 >/proc/sys/kernel/nmi_watchdog
# run_my_uniform_workload_on_all_cpus &
# perf stat -a -e r81d0,r01d1,r08d0,r20d1 sleep 5

That run gives the correct answer.

If I just look at CPU0 CPU4 siblings:

CPU0, counter0 leaks N counts to CPU4, counter 0

but at the same time:

CPU4, counter0 leaks N counts to CPU0, counter 0

This is because we have the same event in the same counter AND the workload is uniform, meaning the event (here loads retired) occurs at the same rate on both siblings.

You can test this by measuring only on one HT.

# perf stat -a -C0 -e r81d0,r01d1,r08d0,r20d1 sleep 5

Quoted 1 line

Note that some events, leak more than they count.

Ok, so AFAIU, this particular workload counts correctly just because counters leak the same amount. If so, what happens if you run this exact same workload with the workaround enabled? I read something about a bit more counter multiplexing... or is there a more serious issue?

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine. --

The issue is that the outcoming leaked counts are not compensated by the incoming leaked counts of the sibling thread. With the workaround, corrupting events are always scheduled with an empty sibling counter. This means that their leaked counts are lost. So it is expected to see lower counts with the workaround. Note that this is not a side-effect of the workaround; leaked counts are expected to be lost with nothing measured on the sibling counter in general.

In a second series we intend to re-integrate the counts for counting mode events. The workaround makes this easier because it guarantees that the sibling counter is unused, thus its counts are purely leaked counts and they can be safely re-integrated.

Quoted 8 lines · Maria Dimakopoulou

The issue is that the outcoming leaked counts are not compensated by the incoming leaked counts of the sibling thread. With the workaround, corrupting events are always scheduled with an empty sibling counter. This means that their leaked counts are lost. So it is expected to see lower counts with the workaround. Note that this is not a side-effect of the workaround; leaked counts are expected to be lost with nothing measured on the sibling counter in general.

Quoted 4 lines

In a second series we intend to re-integrate the counts for counting mode events. The workaround makes this easier because it guarantees that the sibling counter is unused, thus its counts are purely leaked counts and they can be safely re-integrated.

IIUC, sounds to me like reintegrating the leaked counts from the unused counter should be part of the workaround too, not a second series...

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine. --

Quoted 14 lines · Borislav Petkov

Quoted 12 lines · Maria Dimakopoulou

The issue is that the outcoming leaked counts are not compensated by the incoming leaked counts of the sibling thread. With the workaround, corrupting events are always scheduled with an empty sibling counter. This means that their leaked counts are lost. So it is expected to see lower counts with the workaround. Note that this is not a side-effect of the workaround; leaked counts are expected to be lost with nothing measured on the sibling counter in general.

In a second series we intend to re-integrate the counts for counting mode events. The workaround makes this easier because it guarantees that the sibling counter is unused, thus its counts are purely leaked counts and they can be safely re-integrated.

Quoted 3 lines

IIUC, sounds to me like reintegrating the leaked counts from the unused counter should be part of the workaround too, not a second series...

This series aims to avoid corruption of non-corrupting events. Re-integration of the counts is not related to this. This is why we chose to fix this other problem on a second series to keep things clean and concepts separated.

Quoted 6 lines

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine. --

This series aims to avoid corruption of non-corrupting events. Re-integration of the counts is not related to this.

Quoted 2 lines · Maria Dimakopoulou

This series aims to avoid corruption of non-corrupting events. Re-integration of the counts is not related to this.

What do you mean "not related"? Then you have a partial workaround. This whole thing is trying to fix a hw bug so of course it is related.

Once you do the "full" workaround, along with the part which reintegrates the counts and thus completely fixes the issue (I believe), then you don't need to disable it at all because disabling it doesn't make any sense then. See what I'm saying?

This is the whole point Matt and I are trying to make: if the *full* workaround doesn't have any noticeable disadvantages, then you don't need to add a disable-mechanism due to the can of worms opening if you do.

How you get this upstream to ease the review is a whole another story and I applaud your desire to keep things clean.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine. --

As Stephane pointed out, the sysfs entry is optional and the workaround can be disabled only as root.

It is not absolutely necessary and it's not important. We will drop it in V2.

Quoted 5 lines

As Stephane pointed out, the sysfs entry is optional and the workaround can be disabled only as root.

It is not absolutely necessary and it's not important. We will drop it in V2.

I would prefer to keep it. It's fairly complex and it's always good to have a way to disable complex things in case something goes wrong.

Quoted 2 lines · Stephane Eranian

How would you know that you have a uniform workload from inside the kernel?

That's what I'm asking you ;-)

Quoted 5 lines

Quoted 5 lines

Does cpu_sibling_map not give you some indication of whether HT is enabled? I think the topology_thread_cpumask() is the topology API for that. But I could most definitely be wrong. Hopefully someone on the Cc list will know.

Quoted 2 lines

Remember trying some of that, but when perf_event is initialized, those masks are not yet setup properly.

Oh, bummer.

Quoted 11 lines · Matt Fleming

Quoted 2 lines · Stephane Eranian

How would you know that you have a uniform workload from inside the kernel?

That's what I'm asking you ;-)

Quoted 5 lines

Quoted 5 lines

Does cpu_sibling_map not give you some indication of whether HT is enabled? I think the topology_thread_cpumask() is the topology API for that. But I could most definitely be wrong. Hopefully someone on the Cc list will know.

Quoted 4 lines

Quoted 2 lines

Remember trying some of that, but when perf_event is initialized, those masks are not yet setup properly.

Oh, bummer.

So we init perf very early to get nmi-watchdog up and running, but there's no reason you cannot register a second initcall later and flip the switch from it there.

Quoted 15 lines · Peter Zijlstra

Quoted 14 lines · Matt Fleming

Quoted 2 lines · Stephane Eranian

How would you know that you have a uniform workload from inside the kernel?

That's what I'm asking you ;-)

Quoted 7 lines

Quoted 5 lines

Does cpu_sibling_map not give you some indication of whether HT is enabled? I think the topology_thread_cpumask() is the topology API for that. But I could most definitely be wrong. Hopefully someone on the Cc list will know.

Remember trying some of that, but when perf_event is initialized, those masks are not yet setup properly.

Quoted 5 lines

Quoted 1 line

Oh, bummer.

So we init perf very early to get nmi-watchdog up and running, but there's no reason you cannot register a second initcall later and flip the switch from it there.

and what initcall would that be?

Quoted 18 lines · Stephane Eranian

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Matt Fleming

Quoted 2 lines · Stephane Eranian

How would you know that you have a uniform workload from inside the kernel?

That's what I'm asking you ;-)

Quoted 7 lines

Quoted 5 lines

Does cpu_sibling_map not give you some indication of whether HT is enabled? I think the topology_thread_cpumask() is the topology API for that. But I could most definitely be wrong. Hopefully someone on the Cc list will know.

Remember trying some of that, but when perf_event is initialized, those masks are not yet setup properly.

Oh, bummer.

Quoted 5 lines

Quoted 3 lines

So we init perf very early to get nmi-watchdog up and running, but there's no reason you cannot register a second initcall later and flip the switch from it there.

and what initcall would that be?

Pretty much anything !early_initcall() is ran after SMP bringup iirc.

Quoted 1 line

Pretty much anything !early_initcall() is ran after SMP bringup iirc.

Ok, we can try this. Need to check the impact on NMI watchdog if already active.

If you know what you are doing (poweruser), then there are measurements which works fine with the HT erratum. This is why we have the option.

For instance if you only measure events 4x4 in system-wide mode and you know which counters these event are going to use, you don't need the workaround. For instance:

# perf stat -a -e r81d0,r01d1,r08d0,r20d1 sleep 5

Works well if you have a uniform workload across all CPUs. All those events leak, but the leaks balance themselves and you get the correct counts in the end. The advantage is that you don't have to multiplex. With the workaround enable, this would multiplex a lot.

But as I said, this is for experts only.

Quoted 15 lines · Stephane Eranian

If you know what you are doing (poweruser), then there are measurements which works fine with the HT erratum. This is why we have the option.

For instance if you only measure events 4x4 in system-wide mode and you know which counters these event are going to use, you don't need the workaround. For instance:

# perf stat -a -e r81d0,r01d1,r08d0,r20d1 sleep 5

Works well if you have a uniform workload across all CPUs. All those events leak, but the leaks balance themselves and you get the correct counts in the end. The advantage is that you don't have to multiplex. With the workaround enable, this would multiplex a lot.

Quoted 1 line

But as I said, this is for experts only.

Still seems tricky, you really want those pinned to make that guarantee, and even then its a stretch. I don't think perf tool exposes the pinned attribute though, or I'm just not looking right.

I say stretch, because while I think it'll work out and we'll end up programming the counters the same way on each cpu, we really do not make that guarantee either, pinned or not.

I think I agree with Matt in that exposing this to userspace is really asking for trouble.

Now, I've not yet read through the entire patch series, but how impossible is it to allow programming the exact same event on both HT siblings?

There is no HW or FW workaround for this problem.

Quoted 1 line

There is no HW or FW workaround for this problem.

Actually there is for global measurements:

measure with Any bit set and divide by two.

Please add a check that the any bit is set, and disable the workaround for that case.