linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] x86, perf: Use a new PMU ack sequence
@ 2015-10-21 20:16 Andi Kleen
  2015-10-21 20:16 ` [PATCH 2/3] x86, perf: Factor out BTS enable/disable functions Andi Kleen
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Andi Kleen @ 2015-10-21 20:16 UTC (permalink / raw)
  To: peterz; +Cc: linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

The SKL PMU code had a problem with LBR freezing. When a counter
overflows already in the PMI handler, the LBR would be frozen
early and not be unfrozen until the next PMI. This means we would
get stale LBR information.

Depending on the workload this could happen for a few percent
of the PMIs for cycles in adaptive frequency mode, because the frequency
algorithm regularly goes down to very low periods.

This patch implements a new PMU ack sequence that avoids this problem.
The new sequence is:

- (counters are disabled with GLOBAL_CTRL)
There should be no further increments of the counters by later instructions; and
thus no additional PMI (and thus no additional freezing).

- ack the APIC

Clear the APIC PMI LVT entry so that any later interrupt is delivered and is
not lost due to the PMI LVT entry being masked. A lost PMI interrupt could lead to
LBRs staying frozen without entering the PMI handler

- Ack the PMU counters. This unfreezes the LBRs on Skylake (but not
on earlier CPUs which rely on DEBUGCTL writes for this)

- Reenable counters

The WRMSR will start the counters counting again (and will be ordered after the
APIC LVT PMI entry write since WRMSR is architecturally serializing). Since the
APIC PMI LVT is unmasked, any PMI which is caused by these perfmon counters
will trigger an NMI (but the delivery may be delayed until after the next
IRET)

One side effect is that the old retry loop is not possible anymore,
as the counters stay unacked for the majority of the PMI handler,
but that is not a big loss, as "profiling" the PMI was always
a bit dubious. For the old ack sequence it is still supported.

The new sequence is now used unconditionally on all Intel Core/Atom
CPUs. The old irq loop check is removed. Instead we rely on the
generic nmi maximum duration check in the NMI code, together with the perf
enforcement of the maximum sampling rate, to stop any runaway
counters. We also assume that any counter can be stopped by setting
a sufficiently large overflow value.

v2:
Use new ack sequence unconditionally. Remove pmu reset code.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.h       |  1 -
 arch/x86/kernel/cpu/perf_event_intel.c | 82 +++++-----------------------------
 2 files changed, 11 insertions(+), 72 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 499f533..6388540 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -550,7 +550,6 @@ struct x86_pmu {
 	struct event_constraint *event_constraints;
 	struct x86_pmu_quirk *quirks;
 	int		perfctr_second_write;
-	bool		late_ack;
 	unsigned	(*limit_period)(struct perf_event *event, unsigned l);
 
 	/*
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index f63360b..68a37b5 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1741,44 +1741,6 @@ int intel_pmu_save_and_restart(struct perf_event *event)
 	return x86_perf_event_set_period(event);
 }
 
-static void intel_pmu_reset(void)
-{
-	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
-	unsigned long flags;
-	int idx;
-
-	if (!x86_pmu.num_counters)
-		return;
-
-	local_irq_save(flags);
-
-	pr_info("clearing PMU state on CPU#%d\n", smp_processor_id());
-
-	for (idx = 0; idx < x86_pmu.num_counters; idx++) {
-		wrmsrl_safe(x86_pmu_config_addr(idx), 0ull);
-		wrmsrl_safe(x86_pmu_event_addr(idx),  0ull);
-	}
-	for (idx = 0; idx < x86_pmu.num_counters_fixed; idx++)
-		wrmsrl_safe(MSR_ARCH_PERFMON_FIXED_CTR0 + idx, 0ull);
-
-	if (ds)
-		ds->bts_index = ds->bts_buffer_base;
-
-	/* Ack all overflows and disable fixed counters */
-	if (x86_pmu.version >= 2) {
-		intel_pmu_ack_status(intel_pmu_get_status());
-		wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
-	}
-
-	/* Reset LBRs and LBR freezing */
-	if (x86_pmu.lbr_nr) {
-		update_debugctlmsr(get_debugctlmsr() &
-			~(DEBUGCTLMSR_FREEZE_LBRS_ON_PMI|DEBUGCTLMSR_LBR));
-	}
-
-	local_irq_restore(flags);
-}
-
 /*
  * This handler is triggered by the local APIC, so the APIC IRQ handling
  * rules apply:
@@ -1787,39 +1749,22 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 {
 	struct perf_sample_data data;
 	struct cpu_hw_events *cpuc;
-	int bit, loops;
+	int bit;
 	u64 status;
+	u64 orig_status;
 	int handled;
 
 	cpuc = this_cpu_ptr(&cpu_hw_events);
 
-	/*
-	 * No known reason to not always do late ACK,
-	 * but just in case do it opt-in.
-	 */
-	if (!x86_pmu.late_ack)
-		apic_write(APIC_LVTPC, APIC_DM_NMI);
 	__intel_pmu_disable_all();
 	handled = intel_pmu_drain_bts_buffer();
 	handled += intel_bts_interrupt();
 	status = intel_pmu_get_status();
+	orig_status = status;
 	if (!status)
 		goto done;
 
-	loops = 0;
-again:
 	intel_pmu_lbr_read();
-	intel_pmu_ack_status(status);
-	if (++loops > 100) {
-		static bool warned = false;
-		if (!warned) {
-			WARN(1, "perfevents: irq loop stuck!\n");
-			perf_event_print_debug();
-			warned = true;
-		}
-		intel_pmu_reset();
-		goto done;
-	}
 
 	inc_irq_stat(apic_perf_irqs);
 
@@ -1877,22 +1822,20 @@ again:
 			x86_pmu_stop(event, 0);
 	}
 
-	/*
-	 * Repeat if there is more work to be done:
-	 */
-	status = intel_pmu_get_status();
-	if (status)
-		goto again;
-
 done:
-	__intel_pmu_enable_all(0, true);
 	/*
 	 * Only unmask the NMI after the overflow counters
 	 * have been reset. This avoids spurious NMIs on
 	 * Haswell CPUs.
 	 */
-	if (x86_pmu.late_ack)
-		apic_write(APIC_LVTPC, APIC_DM_NMI);
+	apic_write(APIC_LVTPC, APIC_DM_NMI);
+
+	/*
+	 * Ack the PMU late. This avoids bogus freezing
+	 * on Skylake CPUs.
+	 */
+	intel_pmu_ack_status(orig_status);
+	__intel_pmu_enable_all(0, true);
 	return handled;
 }
 
@@ -3455,7 +3398,6 @@ __init int intel_pmu_init(void)
 	case 69: /* 22nm Haswell ULT */
 	case 70: /* 22nm Haswell + GT3e (Intel Iris Pro graphics) */
 		x86_add_quirk(intel_ht_bug);
-		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
 
@@ -3480,7 +3422,6 @@ __init int intel_pmu_init(void)
 	case 86: /* 14nm Broadwell Xeon D */
 	case 71: /* 14nm Broadwell + GT3e (Intel Iris Pro graphics) */
 	case 79: /* 14nm Broadwell Server */
-		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, hsw_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, hsw_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
 
@@ -3513,7 +3454,6 @@ __init int intel_pmu_init(void)
 
 	case 78: /* 14nm Skylake Mobile */
 	case 94: /* 14nm Skylake Desktop */
-		x86_pmu.late_ack = true;
 		memcpy(hw_cache_event_ids, skl_hw_cache_event_ids, sizeof(hw_cache_event_ids));
 		memcpy(hw_cache_extra_regs, skl_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));
 		intel_pmu_lbr_init_skl();
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/3] x86, perf: Factor out BTS enable/disable functions
  2015-10-21 20:16 [PATCH 1/3] x86, perf: Use a new PMU ack sequence Andi Kleen
@ 2015-10-21 20:16 ` Andi Kleen
  2015-10-21 20:16 ` [PATCH 3/3] perf, x86: Use counter freezing with Arch Perfmon v4 Andi Kleen
  2015-10-21 20:36 ` [PATCH 1/3] x86, perf: Use a new PMU ack sequence Peter Zijlstra
  2 siblings, 0 replies; 6+ messages in thread
From: Andi Kleen @ 2015-10-21 20:16 UTC (permalink / raw)
  To: peterz; +Cc: linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

Move BTS enable/disable into own functions. Used by the next
patch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event_intel.c | 43 ++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 68a37b5..31323cc 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1457,20 +1457,39 @@ static __initconst const u64 slm_hw_cache_event_ids
  },
 };
 
-/*
- * Use from PMIs where the LBRs are already disabled.
- */
-static void __intel_pmu_disable_all(void)
+static inline void intel_pmu_maybe_disable_bts(void)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 
-	wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
-
 	if (test_bit(INTEL_PMC_IDX_FIXED_BTS, cpuc->active_mask))
 		intel_pmu_disable_bts();
 	else
 		intel_bts_disable_local();
+}
+
+static inline void intel_pmu_maybe_enable_bts(void)
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 
+	if (test_bit(INTEL_PMC_IDX_FIXED_BTS, cpuc->active_mask)) {
+		struct perf_event *event =
+			cpuc->events[INTEL_PMC_IDX_FIXED_BTS];
+
+		if (WARN_ON_ONCE(!event))
+			return;
+
+		intel_pmu_enable_bts(event->hw.config);
+	} else
+		intel_bts_enable_local();
+}
+
+/*
+ * Use from PMIs where the LBRs are already disabled.
+ */
+static void __intel_pmu_disable_all(void)
+{
+	wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
+	intel_pmu_maybe_disable_bts();
 	intel_pmu_pebs_disable_all();
 }
 
@@ -1488,17 +1507,7 @@ static void __intel_pmu_enable_all(int added, bool pmi)
 	intel_pmu_lbr_enable_all(pmi);
 	wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL,
 			x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask);
-
-	if (test_bit(INTEL_PMC_IDX_FIXED_BTS, cpuc->active_mask)) {
-		struct perf_event *event =
-			cpuc->events[INTEL_PMC_IDX_FIXED_BTS];
-
-		if (WARN_ON_ONCE(!event))
-			return;
-
-		intel_pmu_enable_bts(event->hw.config);
-	} else
-		intel_bts_enable_local();
+	intel_pmu_maybe_enable_bts();
 }
 
 static void intel_pmu_enable_all(int added)
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/3] perf, x86: Use counter freezing with Arch Perfmon v4
  2015-10-21 20:16 [PATCH 1/3] x86, perf: Use a new PMU ack sequence Andi Kleen
  2015-10-21 20:16 ` [PATCH 2/3] x86, perf: Factor out BTS enable/disable functions Andi Kleen
@ 2015-10-21 20:16 ` Andi Kleen
  2015-10-21 20:36 ` [PATCH 1/3] x86, perf: Use a new PMU ack sequence Peter Zijlstra
  2 siblings, 0 replies; 6+ messages in thread
From: Andi Kleen @ 2015-10-21 20:16 UTC (permalink / raw)
  To: peterz; +Cc: linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

Now that we have reliable LBR unfreezing we can also use
real counter freezing.

Arch Perfmon v4 has an improved counter freezing implementation.

With counter freezing the PMU automatically "freezes" all counters
on a counter overflow, so that the PMI handler does not need to
explicitly disable the counter.

With arch perfmon 4 the freeze bits are explicit bits in GLOBAL_STATUS
now, which avoids a range of races in the previous implementation,
and also allows avoiding an extra write to GLOBAL_CTRL.

Advantages of counter freezes:
- It avoids a couple of costly extra MSR writes in the PMI handler
- It makes the PMI handler more accurate, as all counters get
frozen atomically as soon as any counter overflows. So there is
much less counting of the PMI handler itself.

With the freezing we don't need to disable or enable counters or PEBS. Only
BTS which does not support auto-freezing still needs to be explicitly
disabled.

The "status counter ack" serves as the reenable action, by clearing
the freeze bits in the status register.

So previously for a PEBS counter the PMI would do (each line a MSR access)

disable global ctrl
disable pebs
read status
ack status
read status again
reenable global ctrl
reenable pebs

(5x WRMSR, 2x RDMSR)

With the new counter freezing support this is simplified to:

read status
ack

(1x WRMSR, 1x RDMSR)

One issue is that the hardware doesn't like changing the period in
freeze mode. To avoid any issues here we only use counter freezing
when all counters are not in frequency mode, but have a fixed period.
This is kept track of with new per CPU state.

In frequency mode the old mode is still used.

Performance:

When profiling a kernel build on Skylake with different perf options,
measuring the length of all NMI handlers using the nmi handler trace point:

(lower is better)

perf options    `           avg     max   delta
-                           962   37248
-c 100000                   445   31217   -53%  with counter freezing
-g                         1753   47312
-g -c 100000                966   33698   -44%  with counter freezing
--call-graph lbr           3770   37164
--call-graph lbr -c 100000 2433   36930   -35%  with counter freezing
--c.g. dwarf               2055   33598
--c.g. dwarf -c 100000     1478   30491   -28%  with counter freezing

So the average cost of a NMI handler is cut down significantly with
freezing.

At least on this workload this makes -g competitive with the previous
non -g.

The max cost isn't really improved, since that is dominated by
other overhead (such as frequency changes)

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/msr-index.h       |  1 +
 arch/x86/kernel/cpu/perf_event.h       |  5 +++
 arch/x86/kernel/cpu/perf_event_intel.c | 76 +++++++++++++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index b8c14bb..3dc12f9 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -140,6 +140,7 @@
 #define DEBUGCTLMSR_BTS_OFF_OS		(1UL <<  9)
 #define DEBUGCTLMSR_BTS_OFF_USR		(1UL << 10)
 #define DEBUGCTLMSR_FREEZE_LBRS_ON_PMI	(1UL << 11)
+#define DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI (1UL << 12)
 
 #define MSR_PEBS_FRONTEND		0x000003f7
 
diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 6388540..d871c94 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -240,6 +240,11 @@ struct cpu_hw_events {
 	int excl_thread_id; /* 0 or 1 */
 
 	/*
+	 * Counter freezing state.
+	 */
+	int				frozen_enabled;
+
+	/*
 	 * AMD specific bits
 	 */
 	struct amd_nb			*amd_nb;
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 31323cc..f8aab48 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1596,6 +1596,23 @@ static void intel_pmu_nhm_enable_all(int added)
 	intel_pmu_enable_all(added);
 }
 
+static inline bool event_can_freeze(struct perf_event *event)
+{
+	return x86_pmu.version >= 4 && !event->attr.freq;
+}
+
+static void enable_counter_freeze(void)
+{
+	update_debugctlmsr(get_debugctlmsr() |
+			DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI);
+}
+
+static void disable_counter_freeze(void)
+{
+	update_debugctlmsr(get_debugctlmsr() &
+			~DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI);
+}
+
 static inline u64 intel_pmu_get_status(void)
 {
 	u64 status;
@@ -1649,6 +1666,14 @@ static void intel_pmu_disable_event(struct perf_event *event)
 	if (needs_branch_stack(event))
 		intel_pmu_lbr_disable(event);
 
+	/*
+	 * We could disable freezing here, but doesn't hurt if it's on.
+	 * perf remembers the state, and someone else will likely
+	 * reinitialize.
+	 *
+	 * This avoids an extra MSR write in many situations.
+	 */
+
 	if (unlikely(hwc->config_base == MSR_ARCH_PERFMON_FIXED_CTR_CTRL)) {
 		intel_pmu_disable_fixed(hwc);
 		return;
@@ -1715,6 +1740,26 @@ static void intel_pmu_enable_event(struct perf_event *event)
 	if (event->attr.exclude_guest)
 		cpuc->intel_ctrl_host_mask |= (1ull << hwc->idx);
 
+	if (x86_pmu.version >= 4) {
+		/*
+		 * Enable freezing if this event is suitable for freezing,
+		 * and no other event is in frequency mode.
+		 * Otherwise disable freezing for everyone.
+		 */
+		if (event_can_freeze(event) && event->ctx->nr_freq == 0) {
+			if (!cpuc->frozen_enabled) {
+				enable_counter_freeze();
+				cpuc->frozen_enabled = 1;
+			}
+		} else if (cpuc->frozen_enabled) {
+			/* Disable freezing if it's on */
+			intel_pmu_disable_all();
+			cpuc->frozen_enabled = 0;
+			disable_counter_freeze();
+			intel_pmu_enable_all(0);
+		}
+	}
+
 	if (unlikely(event_is_checkpointed(event)))
 		cpuc->intel_cp_status |= (1ull << hwc->idx);
 
@@ -1762,10 +1807,22 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 	u64 status;
 	u64 orig_status;
 	int handled;
+	bool freeze;
 
 	cpuc = this_cpu_ptr(&cpu_hw_events);
 
-	__intel_pmu_disable_all();
+	/*
+	 * With counter freezing the CPU freezes counters on PMI.
+	 * This makes measurements more accurate and generally has
+	 * lower overhead, as we need to change less registers.
+	 *
+	 * We only freeze when all events are in fixed period mode.
+	 */
+	freeze = cpuc->frozen_enabled > 0;
+	if (!freeze)
+		__intel_pmu_disable_all();
+	else
+		intel_pmu_maybe_disable_bts();
 	handled = intel_pmu_drain_bts_buffer();
 	handled += intel_bts_interrupt();
 	status = intel_pmu_get_status();
@@ -1844,7 +1901,10 @@ done:
 	 * on Skylake CPUs.
 	 */
 	intel_pmu_ack_status(orig_status);
-	__intel_pmu_enable_all(0, true);
+	if (!freeze)
+		__intel_pmu_enable_all(0, true);
+	else
+		intel_pmu_maybe_enable_bts();
 	return handled;
 }
 
@@ -2833,6 +2893,11 @@ static void intel_pmu_cpu_dying(int cpu)
 	free_excl_cntrs(cpu);
 
 	fini_debug_store_on_cpu(cpu);
+
+	if (cpuc->frozen_enabled) {
+		cpuc->frozen_enabled = 0;
+		disable_counter_freeze();
+	}
 }
 
 static void intel_pmu_sched_task(struct perf_event_context *ctx,
@@ -3567,6 +3632,13 @@ __init int intel_pmu_init(void)
 		pr_cont("full-width counters, ");
 	}
 
+	/*
+	 * For arch perfmon 4 use counter freezing to avoid
+	 * several MSR accesses in the PMI.
+	 */
+	if (x86_pmu.version >= 4)
+		pr_cont("counter freezing, ");
+
 	return 0;
 }
 
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] x86, perf: Use a new PMU ack sequence
  2015-10-21 20:16 [PATCH 1/3] x86, perf: Use a new PMU ack sequence Andi Kleen
  2015-10-21 20:16 ` [PATCH 2/3] x86, perf: Factor out BTS enable/disable functions Andi Kleen
  2015-10-21 20:16 ` [PATCH 3/3] perf, x86: Use counter freezing with Arch Perfmon v4 Andi Kleen
@ 2015-10-21 20:36 ` Peter Zijlstra
  2015-10-21 21:46   ` Andi Kleen
  2 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2015-10-21 20:36 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel, Andi Kleen, Ingo Molnar

On Wed, Oct 21, 2015 at 01:16:06PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> The SKL PMU code had a problem with LBR freezing. When a counter
> overflows already in the PMI handler, the LBR would be frozen
> early and not be unfrozen until the next PMI. This means we would
> get stale LBR information.
> 
> Depending on the workload this could happen for a few percent
> of the PMIs for cycles in adaptive frequency mode, because the frequency
> algorithm regularly goes down to very low periods.
> 
> This patch implements a new PMU ack sequence that avoids this problem.
> The new sequence is:
> 
> - (counters are disabled with GLOBAL_CTRL)
> There should be no further increments of the counters by later instructions; and
> thus no additional PMI (and thus no additional freezing).
> 
> - ack the APIC
> 
> Clear the APIC PMI LVT entry so that any later interrupt is delivered and is
> not lost due to the PMI LVT entry being masked. A lost PMI interrupt could lead to
> LBRs staying frozen without entering the PMI handler
> 
> - Ack the PMU counters. This unfreezes the LBRs on Skylake (but not
> on earlier CPUs which rely on DEBUGCTL writes for this)
> 
> - Reenable counters
> 
> The WRMSR will start the counters counting again (and will be ordered after the
> APIC LVT PMI entry write since WRMSR is architecturally serializing). Since the
> APIC PMI LVT is unmasked, any PMI which is caused by these perfmon counters
> will trigger an NMI (but the delivery may be delayed until after the next
> IRET)
> 
> One side effect is that the old retry loop is not possible anymore,
> as the counters stay unacked for the majority of the PMI handler,
> but that is not a big loss, as "profiling" the PMI was always
> a bit dubious. For the old ack sequence it is still supported.
> 
> The new sequence is now used unconditionally on all Intel Core/Atom
> CPUs. The old irq loop check is removed. Instead we rely on the
> generic nmi maximum duration check in the NMI code, together with the perf
> enforcement of the maximum sampling rate, to stop any runaway
> counters. We also assume that any counter can be stopped by setting
> a sufficiently large overflow value.
> 
> v2:
> Use new ack sequence unconditionally. Remove pmu reset code.

So this is not something we can easily revert if things go bad. Esp.
since you build on it with the next patches.

Also, teach your editor to wrap at 72 chars for Changelogs.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] x86, perf: Use a new PMU ack sequence
  2015-10-21 20:36 ` [PATCH 1/3] x86, perf: Use a new PMU ack sequence Peter Zijlstra
@ 2015-10-21 21:46   ` Andi Kleen
  2015-10-22  9:25     ` Ingo Molnar
  0 siblings, 1 reply; 6+ messages in thread
From: Andi Kleen @ 2015-10-21 21:46 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Andi Kleen, linux-kernel, Ingo Molnar

> > v2:
> > Use new ack sequence unconditionally. Remove pmu reset code.
> 
> So this is not something we can easily revert if things go bad. Esp.
> since you build on it with the next patches.

Ok, and?

You want me to go back to the previous patch? That one is easily
undoable (just disable the flag for the model)

Another alternative would be to fork the PMI handler into a new and
an old version, that is switchable.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] x86, perf: Use a new PMU ack sequence
  2015-10-21 21:46   ` Andi Kleen
@ 2015-10-22  9:25     ` Ingo Molnar
  0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2015-10-22  9:25 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Peter Zijlstra, Andi Kleen, linux-kernel,
	Arnaldo Carvalho de Melo, Thomas Gleixner


* Andi Kleen <ak@linux.intel.com> wrote:

> > > v2:
> > > Use new ack sequence unconditionally. Remove pmu reset code.
> > 
> > So this is not something we can easily revert if things go bad. Esp.
> > since you build on it with the next patches.
> 
> Ok, and?

Sigh, you are being disruptive again.

> You want me to go back to the previous patch? That one is easily
> undoable (just disable the flag for the model)
> 
> Another alternative would be to fork the PMI handler into a new and
> an old version, that is switchable.

Here you pretend that you didn't read the sane solution that was suggested to you 
just three days ago:

  http://lkml.kernel.org/r/20151019070812.GB17855@gmail.com

  " > > Ingo, do you want to first merge the safe patch and then clean up?
    >
    > Yeah, would be nice to structure it that way, out of general paranoia.
  "

I.e. first apply the safe approach, then, after the dependent changes, clean it up 
by introducing the dangerous change.

The thing is, I'm close to summarily NAK-ing any patches from you to the perf 
subsystem, due to the unacceptably low quality patches combined with obtuse 
passive-aggressive obstruction you are routinely burdening maintainers with.

Btw., I noticed that you routinely don't Cc me to perf patches. Please always Cc: 
me to perf patches (both kernel and tooling patches). I still will not apply them 
directly, only if another perf maintainer signs off on them, but I'd like to have 
a record of all your submissions.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-10-22  9:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-21 20:16 [PATCH 1/3] x86, perf: Use a new PMU ack sequence Andi Kleen
2015-10-21 20:16 ` [PATCH 2/3] x86, perf: Factor out BTS enable/disable functions Andi Kleen
2015-10-21 20:16 ` [PATCH 3/3] perf, x86: Use counter freezing with Arch Perfmon v4 Andi Kleen
2015-10-21 20:36 ` [PATCH 1/3] x86, perf: Use a new PMU ack sequence Peter Zijlstra
2015-10-21 21:46   ` Andi Kleen
2015-10-22  9:25     ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).