Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1645799 > unrolled thread

[PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter kan.liang@intel.com - 2017-05-19 20:00 +0200
    [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86 kan.liang@intel.com - 2017-05-19 20:00 +0200
      Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref  cycles on x86 Peter Zijlstra <peterz@infradead.org> - 2017-05-22 14:10 +0200
        Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref  cycles on x86 Peter Zijlstra <peterz@infradead.org> - 2017-05-22 14:10 +0200
        RE: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to  ref cycles on x86 "Liang, Kan" <kan.liang@intel.com> - 2017-05-22 19:00 +0200
          Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref  cycles on x86 Peter Zijlstra <peterz@infradead.org> - 2017-05-22 21:30 +0200
      Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref  cycles on x86 Stephane Eranian <eranian@google.com> - 2017-05-22 20:30 +0200
        Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref  cycles on x86 Andi Kleen <ak@linux.intel.com> - 2017-05-22 22:10 +0200
    Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Peter Zijlstra <peterz@infradead.org> - 2017-05-22 10:40 +0200
      Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Stephane Eranian <eranian@google.com> - 2017-05-22 20:20 +0200
    Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Peter Zijlstra <peterz@infradead.org> - 2017-05-22 11:30 +0200
      Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Peter Zijlstra <peterz@infradead.org> - 2017-05-22 14:30 +0200
        RE: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter "Liang, Kan" <kan.liang@intel.com> - 2017-05-22 19:00 +0200
      RE: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter "Liang, Kan" <kan.liang@intel.com> - 2017-05-22 19:00 +0200
        Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Peter Zijlstra <peterz@infradead.org> - 2017-05-22 21:30 +0200
          Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Stephane Eranian <eranian@google.com> - 2017-05-22 21:30 +0200
            RE: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter "Liang, Kan" <kan.liang@intel.com> - 2017-05-23 00:00 +0200
            Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Peter Zijlstra <peterz@infradead.org> - 2017-05-23 08:40 +0200
              Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Stephane Eranian <eranian@google.com> - 2017-05-23 08:50 +0200
                Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Andi Kleen <ak@linux.intel.com> - 2017-05-24 17:50 +0200
                  Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP  counter Vince Weaver <vincent.weaver@maine.edu> - 2017-05-24 18:10 +0200
                    Re: [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter Andi Kleen <ak@linux.intel.com> - 2017-05-24 19:00 +0200

Page 1 of 2  [1] 2  Next page →

#1645799 — [PATCH 1/2] perf/x86/intel: enable CPU ref_cycles for GP counter

From: Kan Liang <Kan.liang@intel.com>

The CPU ref_cycles can only be used by one user at the same time,
otherwise a "not counted" error will be displaced.
    [kan]$ sudo perf stat -x, -e ref-cycles,ref-cycles -- sleep 1
    1203264,,ref-cycles,513112,100.00,,,,
    <not counted>,,ref-cycles,0,0.00,,,,

CPU ref_cycles can only be counted by fixed counter 2. It uses
pseudo-encoding. The GP counter doesn't recognize.

BUS_CYCLES (0x013c) is another event which is not affected by core
frequency changes. It has a constant ratio with the CPU ref_cycles.
BUS_CYCLES could be used as an alternative event for ref_cycles on GP
counter.
A hook is implemented in x86_schedule_events. If the fixed counter 2 is
occupied and a GP counter is assigned, BUS_CYCLES is used to replace
ref_cycles. A new flag PERF_X86_EVENT_REF_CYCLES_REP in
hw_perf_event is introduced to indicate the replacement.
To make the switch transparent, counting and sampling are also specially
handled.
 - For counting, it multiplies the result with the constant ratio after
   reading it.
 - For sampling with fixed period, the BUS_CYCLES period = ref_cycles
   period / the constant ratio.
 - For sampling with fixed frequency, the adaptive frequency algorithm
   will figure it out on its own. Do nothing.

The constant ratio is model specific.
For the model after NEHALEM but before Skylake, the ratio is defined in
MSR_PLATFORM_INFO.
For the model after Skylake, it can be get from CPUID.15H.
For Knights Landing, Goldmont and later, the ratio is always 1.

The old Silvermont/Airmont, Core2 and Atom machines are not covered by
the patch. The behavior on those machines will not change.

Signed-off-by: Kan Liang <Kan.liang@intel.com>
---
 arch/x86/events/core.c       | 19 +++++++++
 arch/x86/events/intel/core.c | 93 ++++++++++++++++++++++++++++++++++++++++----
 arch/x86/events/perf_event.h |  3 ++
 3 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 580b60f..e8b2326 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event *event)
 	delta = (new_raw_count << shift) - (prev_raw_count << shift);
 	delta >>= shift;
 
+	/* Correct the count number if applying ref_cycles replacement */
+	if (!is_sampling_event(event) &&
+	    (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
+		delta *= x86_pmu.ref_cycles_factor;
 	local64_add(delta, &event->count);
 	local64_sub(delta, &hwc->period_left);
 
@@ -934,6 +938,21 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
 		for (i = 0; i < n; i++) {
 			e = cpuc->event_list[i];
 			e->hw.flags |= PERF_X86_EVENT_COMMITTED;
+
+			/*
+			 * 0x0300 is pseudo-encoding for REF_CPU_CYCLES.
+			 * It indicates that fixed counter 2 should be used.
+			 *
+			 * If fixed counter 2 is occupied and a GP counter
+			 * is assigned, an alternative event which can be
+			 * counted in GP counter will be used to replace
+			 * the pseudo-encoding REF_CPU_CYCLES event.
+			 */
+			if (((e->hw.config & X86_RAW_EVENT_MASK) == 0x0300) &&
+			    (assign[i] < INTEL_PMC_IDX_FIXED) &&
+			    x86_pmu.ref_cycles_rep)
+				x86_pmu.ref_cycles_rep(e);
+
 			if (x86_pmu.commit_scheduling)
 				x86_pmu.commit_scheduling(cpuc, i, assign[i]);
 		}
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index a6d91d4..af5a464 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3063,6 +3063,55 @@ static unsigned bdw_limit_period(struct perf_event *event, unsigned left)
 	return left;
 }
 
+static __init unsigned int glm_get_ref_cycles_factor(void)
+{
+	return 1;
+}
+
+static __init unsigned int nhm_get_ref_cycles_factor(void)
+{
+	u64 platform_info;
+
+	rdmsrl(MSR_PLATFORM_INFO, platform_info);
+	return (platform_info >> 8) & 0xff;
+}
+
+static __init unsigned int skl_get_ref_cycles_factor(void)
+{
+	unsigned int cpuid21_eax, cpuid21_ebx, cpuid21_ecx, unused;
+
+	cpuid(21, &cpuid21_eax, &cpuid21_ebx, &cpuid21_ecx, &unused);
+	if (!cpuid21_eax || !cpuid21_ebx)
+		return 0;
+
+	return cpuid21_ebx / cpuid21_eax;
+}
+
+/*
+ * BUS_CYCLES (0x013c) is another event which is not affected by core
+ * frequency changes. It has a constant ratio with the CPU ref_cycles.
+ * BUS_CYCLES could be used as an alternative event for ref_cycles on
+ * GP counter.
+ */
+void nhm_ref_cycles_rep(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	hwc->config = (hwc->config & ~X86_RAW_EVENT_MASK) |
+		      intel_perfmon_event_map[PERF_COUNT_HW_BUS_CYCLES];
+	hwc->flags |= PERF_X86_EVENT_REF_CYCLES_REP;
+
+	/* adjust the sample period for ref_cycles replacement event */
+	if (is_sampling_event(event) && hwc->sample_period != 1) {
+
+		hwc->sample_period /= x86_pmu.ref_cycles_factor;
+		if (hwc->sample_period < 2)
+			hwc->sample_period = 2;
+		hwc->last_period = hwc->sample_period;
+		local64_set(&hwc->period_left, hwc->sample_period);
+	}
+}
+
 PMU_FORMAT_ATTR(event,	"config:0-7"	);
 PMU_FORMAT_ATTR(umask,	"config:8-15"	);
 PMU_FORMAT_ATTR(edge,	"config:18"	);
@@ -3712,7 +3761,9 @@ __init int intel_pmu_init(void)
 
 		intel_pmu_pebs_data_source_nhm();
 		x86_add_quirk(intel_nehalem_quirk);
-
+		x86_pmu.ref_cycles_factor = nhm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 		pr_cont("Nehalem events, ");
 		break;
 
@@ -3772,6 +3823,9 @@ __init int intel_pmu_init(void)
 		x86_pmu.lbr_pt_coexist = true;
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.cpu_events = glm_events_attrs;
+		x86_pmu.ref_cycles_factor = glm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 		pr_cont("Goldmont events, ");
 		break;
 
@@ -3801,6 +3855,9 @@ __init int intel_pmu_init(void)
 			X86_CONFIG(.event=0xb1, .umask=0x3f, .inv=1, .cmask=1);
 
 		intel_pmu_pebs_data_source_nhm();
+		x86_pmu.ref_cycles_factor = nhm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 		pr_cont("Westmere events, ");
 		break;
 
@@ -3836,6 +3893,9 @@ __init int intel_pmu_init(void)
 		/* UOPS_DISPATCHED.THREAD,c=1,i=1 to count stall cycles*/
 		intel_perfmon_event_map[PERF_COUNT_HW_STALLED_CYCLES_BACKEND] =
 			X86_CONFIG(.event=0xb1, .umask=0x01, .inv=1, .cmask=1);
+		x86_pmu.ref_cycles_factor = nhm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 
 		pr_cont("SandyBridge events, ");
 		break;
@@ -3870,6 +3930,9 @@ __init int intel_pmu_init(void)
 		/* UOPS_ISSUED.ANY,c=1,i=1 to count stall cycles */
 		intel_perfmon_event_map[PERF_COUNT_HW_STALLED_CYCLES_FRONTEND] =
 			X86_CONFIG(.event=0x0e, .umask=0x01, .inv=1, .cmask=1);
+		x86_pmu.ref_cycles_factor = nhm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 
 		pr_cont("IvyBridge events, ");
 		break;
@@ -3899,6 +3962,9 @@ __init int intel_pmu_init(void)
 		x86_pmu.get_event_constraints = hsw_get_event_constraints;
 		x86_pmu.cpu_events = hsw_events_attrs;
 		x86_pmu.lbr_double_abort = true;
+		x86_pmu.ref_cycles_factor = nhm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 		pr_cont("Haswell events, ");
 		break;
 
@@ -3935,6 +4001,9 @@ __init int intel_pmu_init(void)
 		x86_pmu.get_event_constraints = hsw_get_event_constraints;
 		x86_pmu.cpu_events = hsw_events_attrs;
 		x86_pmu.limit_period = bdw_limit_period;
+		x86_pmu.ref_cycles_factor = nhm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 		pr_cont("Broadwell events, ");
 		break;
 
@@ -3953,6 +4022,9 @@ __init int intel_pmu_init(void)
 		/* all extra regs are per-cpu when HT is on */
 		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
 		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
+		x86_pmu.ref_cycles_factor = glm_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 
 		pr_cont("Knights Landing/Mill events, ");
 		break;
@@ -3988,6 +4060,9 @@ __init int intel_pmu_init(void)
 						  skl_format_attr);
 		WARN_ON(!x86_pmu.format_attrs);
 		x86_pmu.cpu_events = hsw_events_attrs;
+		x86_pmu.ref_cycles_factor = skl_get_ref_cycles_factor();
+		if (x86_pmu.ref_cycles_factor)
+			x86_pmu.ref_cycles_rep = nhm_ref_cycles_rep;
 		pr_cont("Skylake events, ");
 		break;
 
@@ -4024,15 +4099,17 @@ __init int intel_pmu_init(void)
 		((1LL << x86_pmu.num_counters_fixed)-1) << INTEL_PMC_IDX_FIXED;
 
 	if (x86_pmu.event_constraints) {
-		/*
-		 * event on fixed counter2 (REF_CYCLES) only works on this
-		 * counter, so do not extend mask to generic counters
-		 */
 		for_each_event_constraint(c, x86_pmu.event_constraints) {
-			if (c->cmask == FIXED_EVENT_FLAGS
-			    && c->idxmsk64 != INTEL_PMC_MSK_FIXED_REF_CYCLES) {
+			if (c->cmask == FIXED_EVENT_FLAGS)
 				c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
-			}
+			/*
+			 * event on fixed counter2 (REF_CYCLES) only works on
+			 * this counter on some old platforms, e.g. core2, Atom.
+			 * So do not extend mask to generic counters
+			 */
+			if ((c->idxmsk64 == INTEL_PMC_MSK_FIXED_REF_CYCLES) &&
+			    !x86_pmu.ref_cycles_rep)
+				c->idxmsk64 &= ~((1ULL << x86_pmu.num_counters) - 1);
 			c->idxmsk64 &=
 				~(~0ULL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed));
 			c->weight = hweight64(c->idxmsk64);
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index be3d362..6497d0a 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -68,6 +68,7 @@ struct event_constraint {
 #define PERF_X86_EVENT_EXCL_ACCT	0x0200 /* accounted EXCL event */
 #define PERF_X86_EVENT_AUTO_RELOAD	0x0400 /* use PEBS auto-reload */
 #define PERF_X86_EVENT_FREERUNNING	0x0800 /* use freerunning PEBS */
+#define PERF_X86_EVENT_REF_CYCLES_REP	0x1000 /* use ref_cycles replacement */
 
 
 struct amd_nb {
@@ -550,6 +551,8 @@ struct x86_pmu {
 	int		perfctr_second_write;
 	bool		late_ack;
 	unsigned	(*limit_period)(struct perf_event *event, unsigned l);
+	unsigned int	ref_cycles_factor;
+	void		(*ref_cycles_rep)(struct perf_event *event);
 
 	/*
 	 * sysfs attrs
-- 
2.7.4

[toc] | [next] | [standalone]

#1645802 — [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86

From: Kan Liang <Kan.liang@intel.com>

The NMI watchdog uses either the fixed cycles or a generic cycles
counter. This causes a lot of conflicts with users of the PMU who want
to run a full group including the cycles fixed counter, for example the
--topdown support recently added to perf stat. The code needs to fall
back to not use groups, which can cause measurement inaccuracy due to
multiplexing errors.

This patch switches the NMI watchdog to use reference cycles on Intel
systems. This is actually more accurate than cycles, because cycles can
tick faster than the measured CPU Frequency due to Turbo mode.

The ref cycles always tick at their frequency, or slower when the system
is idling. That means the NMI watchdog can never expire too early,
unlike with cycles.

The reference cycles tick roughly at the frequency of the TSC, so the
same period computation can be used.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---

This patch was once merged, but reverted later.
Because ref-cycles can not be used anymore when watchdog is enabled.
The commit is 44530d588e142a96cf0cd345a7cb8911c4f88720

The patch 1/2 has extended the ref-cycles to GP counter. The concern
should be gone.

Rebased the patch and repost.


 arch/x86/kernel/apic/hw_nmi.c | 8 ++++++++
 include/linux/nmi.h           | 1 +
 kernel/watchdog_hld.c         | 7 +++++++
 3 files changed, 16 insertions(+)

diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index c73c9fb..acd21dc 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -18,8 +18,16 @@
 #include <linux/nmi.h>
 #include <linux/init.h>
 #include <linux/delay.h>
+#include <linux/perf_event.h>
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
+int hw_nmi_get_event(void)
+{
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+		return PERF_COUNT_HW_REF_CPU_CYCLES;
+	return PERF_COUNT_HW_CPU_CYCLES;
+}
+
 u64 hw_nmi_get_sample_period(int watchdog_thresh)
 {
 	return (u64)(cpu_khz) * 1000 * watchdog_thresh;
diff --git a/include/linux/nmi.h b/include/linux/nmi.h
index aa3cd08..b2fa444 100644
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -141,6 +141,7 @@ static inline bool trigger_single_cpu_backtrace(int cpu)
 
 #ifdef CONFIG_LOCKUP_DETECTOR
 u64 hw_nmi_get_sample_period(int watchdog_thresh);
+int hw_nmi_get_event(void);
 extern int nmi_watchdog_enabled;
 extern int soft_watchdog_enabled;
 extern int watchdog_user_enabled;
diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
index 54a427d..f899766 100644
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -70,6 +70,12 @@ void touch_nmi_watchdog(void)
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);
 
+/* Can be overridden by architecture */
+__weak int hw_nmi_get_event(void)
+{
+	return PERF_COUNT_HW_CPU_CYCLES;
+}
+
 static struct perf_event_attr wd_hw_attr = {
 	.type		= PERF_TYPE_HARDWARE,
 	.config		= PERF_COUNT_HW_CPU_CYCLES,
@@ -165,6 +171,7 @@ int watchdog_nmi_enable(unsigned int cpu)
 
 	wd_attr = &wd_hw_attr;
 	wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
+	wd_attr->config = hw_nmi_get_event();
 
 	/* Try to register using hardware perf events */
 	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback, NULL);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1646804 — Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86

On Fri, May 19, 2017 at 10:06:22AM -0700, kan.liang@intel.com wrote:
> This patch was once merged, but reverted later.
> Because ref-cycles can not be used anymore when watchdog is enabled.
> The commit is 44530d588e142a96cf0cd345a7cb8911c4f88720
> 
> The patch 1/2 has extended the ref-cycles to GP counter. The concern
> should be gone.

So its not a problem if every Atom prior to Goldmont, and all Core/Core2
products regress?

P6 and P4 you've entirely broken, as they don't have REF_CPU_CYCLES at all.

So no, I don't think this is right even now.

[toc] | [prev] | [next] | [standalone]

#1646809 — Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86

On Mon, May 22, 2017 at 02:03:21PM +0200, Peter Zijlstra wrote:
> On Fri, May 19, 2017 at 10:06:22AM -0700, kan.liang@intel.com wrote:
> > This patch was once merged, but reverted later.
> > Because ref-cycles can not be used anymore when watchdog is enabled.
> > The commit is 44530d588e142a96cf0cd345a7cb8911c4f88720
> > 
> > The patch 1/2 has extended the ref-cycles to GP counter. The concern
> > should be gone.
> 
> So its not a problem if every Atom prior to Goldmont, and all Core/Core2
> products regress?
> 
> P6 and P4 you've entirely broken, as they don't have REF_CPU_CYCLES at all.

 + KNC

> 
> So no, I don't think this is right even now.
> 
> 

[toc] | [prev] | [next] | [standalone]

#1647177 — RE: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86

> On Fri, May 19, 2017 at 10:06:22AM -0700, kan.liang@intel.com wrote:
> > This patch was once merged, but reverted later.
> > Because ref-cycles can not be used anymore when watchdog is enabled.
> > The commit is 44530d588e142a96cf0cd345a7cb8911c4f88720
> >
> > The patch 1/2 has extended the ref-cycles to GP counter. The concern
> > should be gone.
> 
> So its not a problem if every Atom prior to Goldmont, and all Core/Core2
> products regress?
> 
> P6 and P4 you've entirely broken, as they don't have REF_CPU_CYCLES at all.
> 
> So no, I don't think this is right even now.
> 

Right, the patch 1/2 doesn't cover all platforms.
I will only apply the patch for the platforms,
which have ref cycles on GP counters.

Thanks,
Kan

[toc] | [prev] | [next] | [standalone]

#1647288 — Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86

On Mon, May 22, 2017 at 04:58:04PM +0000, Liang, Kan wrote:
> 
> 
> > On Fri, May 19, 2017 at 10:06:22AM -0700, kan.liang@intel.com wrote:
> > > This patch was once merged, but reverted later.
> > > Because ref-cycles can not be used anymore when watchdog is enabled.
> > > The commit is 44530d588e142a96cf0cd345a7cb8911c4f88720
> > >
> > > The patch 1/2 has extended the ref-cycles to GP counter. The concern
> > > should be gone.
> > 
> > So its not a problem if every Atom prior to Goldmont, and all Core/Core2
> > products regress?
> > 
> > P6 and P4 you've entirely broken, as they don't have REF_CPU_CYCLES at all.
> > 
> > So no, I don't think this is right even now.
> > 
> 
> Right, the patch 1/2 doesn't cover all platforms.
> I will only apply the patch for the platforms,
> which have ref cycles on GP counters.

Right, so if you move the weak function into arch/x86/events/core.c and
simply test for x86_pmu.this_ref_alis_function_thing being !NULL that
should all work.

[toc] | [prev] | [next] | [standalone]

#1647243 — Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86

Andi,

On Fri, May 19, 2017 at 10:06 AM,  <kan.liang@intel.com> wrote:
> From: Kan Liang <Kan.liang@intel.com>
>
> The NMI watchdog uses either the fixed cycles or a generic cycles
> counter. This causes a lot of conflicts with users of the PMU who want
> to run a full group including the cycles fixed counter, for example the
> --topdown support recently added to perf stat. The code needs to fall
> back to not use groups, which can cause measurement inaccuracy due to
> multiplexing errors.
>
> This patch switches the NMI watchdog to use reference cycles on Intel
> systems. This is actually more accurate than cycles, because cycles can
> tick faster than the measured CPU Frequency due to Turbo mode.
>
You have not addressed why you need that accuracy?
This is about detecting hard deadlocks, so you don't care about a few seconds
accuracy. Instead of introducing all this complexity, why not simply extend the
period of the watchdog to be more tolerant to Turbo scaling t o avoid
false positive
and continue to use core-cycles, an event universally available.


> The ref cycles always tick at their frequency, or slower when the system
> is idling. That means the NMI watchdog can never expire too early,
> unlike with cycles.
>
Just make the period longer, like 30% longer. Take the max turbo factor you can
get and use that. It is okay if it takes longer of machine with
smaller max Turbo ratios.

What is the problem with this approach instead?

> The reference cycles tick roughly at the frequency of the TSC, so the
> same period computation can be used.
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>
> This patch was once merged, but reverted later.
> Because ref-cycles can not be used anymore when watchdog is enabled.
> The commit is 44530d588e142a96cf0cd345a7cb8911c4f88720
>
> The patch 1/2 has extended the ref-cycles to GP counter. The concern
> should be gone.
>
> Rebased the patch and repost.
>
>
>  arch/x86/kernel/apic/hw_nmi.c | 8 ++++++++
>  include/linux/nmi.h           | 1 +
>  kernel/watchdog_hld.c         | 7 +++++++
>  3 files changed, 16 insertions(+)
>
> diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> index c73c9fb..acd21dc 100644
> --- a/arch/x86/kernel/apic/hw_nmi.c
> +++ b/arch/x86/kernel/apic/hw_nmi.c
> @@ -18,8 +18,16 @@
>  #include <linux/nmi.h>
>  #include <linux/init.h>
>  #include <linux/delay.h>
> +#include <linux/perf_event.h>
>
>  #ifdef CONFIG_HARDLOCKUP_DETECTOR
> +int hw_nmi_get_event(void)
> +{
> +       if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +               return PERF_COUNT_HW_REF_CPU_CYCLES;
> +       return PERF_COUNT_HW_CPU_CYCLES;
> +}
> +
>  u64 hw_nmi_get_sample_period(int watchdog_thresh)
>  {
>         return (u64)(cpu_khz) * 1000 * watchdog_thresh;
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index aa3cd08..b2fa444 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -141,6 +141,7 @@ static inline bool trigger_single_cpu_backtrace(int cpu)
>
>  #ifdef CONFIG_LOCKUP_DETECTOR
>  u64 hw_nmi_get_sample_period(int watchdog_thresh);
> +int hw_nmi_get_event(void);
>  extern int nmi_watchdog_enabled;
>  extern int soft_watchdog_enabled;
>  extern int watchdog_user_enabled;
> diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
> index 54a427d..f899766 100644
> --- a/kernel/watchdog_hld.c
> +++ b/kernel/watchdog_hld.c
> @@ -70,6 +70,12 @@ void touch_nmi_watchdog(void)
>  }
>  EXPORT_SYMBOL(touch_nmi_watchdog);
>
> +/* Can be overridden by architecture */
> +__weak int hw_nmi_get_event(void)
> +{
> +       return PERF_COUNT_HW_CPU_CYCLES;
> +}
> +
>  static struct perf_event_attr wd_hw_attr = {
>         .type           = PERF_TYPE_HARDWARE,
>         .config         = PERF_COUNT_HW_CPU_CYCLES,
> @@ -165,6 +171,7 @@ int watchdog_nmi_enable(unsigned int cpu)
>
>         wd_attr = &wd_hw_attr;
>         wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
> +       wd_attr->config = hw_nmi_get_event();
>
>         /* Try to register using hardware perf events */
>         event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback, NULL);
> --
> 2.7.4
>

[toc] | [prev] | [next] | [standalone]

#1647334 — Re: [PATCH 2/2] perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86

> 
> > The ref cycles always tick at their frequency, or slower when the system
> > is idling. That means the NMI watchdog can never expire too early,
> > unlike with cycles.
> >
> Just make the period longer, like 30% longer. Take the max turbo factor you can
> get and use that. It is okay if it takes longer of machine with
> smaller max Turbo ratios.

That would be a ticking time bomb. Turbo ratios are likely to grow.

There's no architectural way to get a turbo factor. Even if there
is a model specific MSR it would likely not work in virtualization etc.

You could make the timeout ridiculously large, but even that would
have a small chance of failure.

-Andi

[toc] | [prev] | [next] | [standalone]

#1646605

On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> From: Kan Liang <Kan.liang@intel.com>
> 
> The CPU ref_cycles can only be used by one user at the same time,
> otherwise a "not counted" error will be displaced.
>     [kan]$ sudo perf stat -x, -e ref-cycles,ref-cycles -- sleep 1
>     1203264,,ref-cycles,513112,100.00,,,,
>     <not counted>,,ref-cycles,0,0.00,,,,
> 
> CPU ref_cycles can only be counted by fixed counter 2. It uses
> pseudo-encoding. The GP counter doesn't recognize.
> 
> BUS_CYCLES (0x013c) is another event which is not affected by core
> frequency changes. It has a constant ratio with the CPU ref_cycles.
> BUS_CYCLES could be used as an alternative event for ref_cycles on GP
> counter.
> A hook is implemented in x86_schedule_events. If the fixed counter 2 is
> occupied and a GP counter is assigned, BUS_CYCLES is used to replace
> ref_cycles. A new flag PERF_X86_EVENT_REF_CYCLES_REP in
> hw_perf_event is introduced to indicate the replacement.
> To make the switch transparent, counting and sampling are also specially
> handled.
>  - For counting, it multiplies the result with the constant ratio after
>    reading it.
>  - For sampling with fixed period, the BUS_CYCLES period = ref_cycles
>    period / the constant ratio.
>  - For sampling with fixed frequency, the adaptive frequency algorithm
>    will figure it out on its own. Do nothing.
> 
> The constant ratio is model specific.
> For the model after NEHALEM but before Skylake, the ratio is defined in
> MSR_PLATFORM_INFO.
> For the model after Skylake, it can be get from CPUID.15H.
> For Knights Landing, Goldmont and later, the ratio is always 1.
> 
> The old Silvermont/Airmont, Core2 and Atom machines are not covered by
> the patch. The behavior on those machines will not change.

Maybe I missed it, but *why* are we doing this?

[toc] | [prev] | [next] | [standalone]

#1647235

Hi,

On Mon, May 22, 2017 at 1:30 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
>> From: Kan Liang <Kan.liang@intel.com>
>>
>> The CPU ref_cycles can only be used by one user at the same time,
>> otherwise a "not counted" error will be displaced.
>>     [kan]$ sudo perf stat -x, -e ref-cycles,ref-cycles -- sleep 1
>>     1203264,,ref-cycles,513112,100.00,,,,
>>     <not counted>,,ref-cycles,0,0.00,,,,
>>
>> CPU ref_cycles can only be counted by fixed counter 2. It uses
>> pseudo-encoding. The GP counter doesn't recognize.
>>
>> BUS_CYCLES (0x013c) is another event which is not affected by core
>> frequency changes. It has a constant ratio with the CPU ref_cycles.
>> BUS_CYCLES could be used as an alternative event for ref_cycles on GP
>> counter.
>> A hook is implemented in x86_schedule_events. If the fixed counter 2 is
>> occupied and a GP counter is assigned, BUS_CYCLES is used to replace
>> ref_cycles. A new flag PERF_X86_EVENT_REF_CYCLES_REP in
>> hw_perf_event is introduced to indicate the replacement.
>> To make the switch transparent, counting and sampling are also specially
>> handled.
>>  - For counting, it multiplies the result with the constant ratio after
>>    reading it.
>>  - For sampling with fixed period, the BUS_CYCLES period = ref_cycles
>>    period / the constant ratio.
>>  - For sampling with fixed frequency, the adaptive frequency algorithm
>>    will figure it out on its own. Do nothing.
>>
>> The constant ratio is model specific.
>> For the model after NEHALEM but before Skylake, the ratio is defined in
>> MSR_PLATFORM_INFO.
>> For the model after Skylake, it can be get from CPUID.15H.
>> For Knights Landing, Goldmont and later, the ratio is always 1.
>>
>> The old Silvermont/Airmont, Core2 and Atom machines are not covered by
>> the patch. The behavior on those machines will not change.
>
> Maybe I missed it, but *why* are we doing this?

Yes, I would like to understand the motivation for this added
complexity as well.

My guess is that you have a situation where ref-cycles is used
constantly, i.e., pinned, and therefore you
lose the ability to count it for any other user. This is the case when
you switch the hard lockup detector
(NMI watchdog) to using ref-cycles instead of core cycles. This is
what you are doing in patch 2/2 actually.
Another scenario could be with virtual machines. KVM makes all guests
events use pinned events on the host. So if the guest is measuring
ref-cycles, then the host cannot.Well, I am hoping this is not the
case because as far as I remember system-wide pinned has
higher priority than per-process pinned.

You cannot make your change transparent in sampling mode. You are
adjusting the period with the ratio. If
the user asks for the period to be recorded in each sample, the
modified period will be captured. If I say I
want to sample every 1M ref-cycles  and I set event_attr.sample_type =
PERF_SAMPLE_PERIOD, then I
expect to see 1M in each sample and not some scaled value. So you need
to address this problem, including
in frequency mode.

[toc] | [prev] | [next] | [standalone]

#1646655

On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 580b60f..e8b2326 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event *event)
>  	delta = (new_raw_count << shift) - (prev_raw_count << shift);
>  	delta >>= shift;
>  
> +	/* Correct the count number if applying ref_cycles replacement */
> +	if (!is_sampling_event(event) &&
> +	    (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
> +		delta *= x86_pmu.ref_cycles_factor;

That condition seems wrong, why only correct for !sampling events?

>  	local64_add(delta, &event->count);
>  	local64_sub(delta, &hwc->period_left);
>  


> @@ -934,6 +938,21 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)
>  		for (i = 0; i < n; i++) {
>  			e = cpuc->event_list[i];
>  			e->hw.flags |= PERF_X86_EVENT_COMMITTED;
> +
> +			/*
> +			 * 0x0300 is pseudo-encoding for REF_CPU_CYCLES.
> +			 * It indicates that fixed counter 2 should be used.
> +			 *
> +			 * If fixed counter 2 is occupied and a GP counter
> +			 * is assigned, an alternative event which can be
> +			 * counted in GP counter will be used to replace
> +			 * the pseudo-encoding REF_CPU_CYCLES event.
> +			 */
> +			if (((e->hw.config & X86_RAW_EVENT_MASK) == 0x0300) &&
> +			    (assign[i] < INTEL_PMC_IDX_FIXED) &&
> +			    x86_pmu.ref_cycles_rep)
> +				x86_pmu.ref_cycles_rep(e);
> +
>  			if (x86_pmu.commit_scheduling)
>  				x86_pmu.commit_scheduling(cpuc, i, assign[i]);
>  		}

This looks dodgy, this is the branch were we managed to schedule all
events. Why would we need to consider anything here?

I was expecting a retry if there are still unscheduled events and one of
the events was our 0x0300 event. In that case you have to reset the
event and retry the whole scheduling thing.

[toc] | [prev] | [next] | [standalone]

#1646830

On Mon, May 22, 2017 at 11:19:16AM +0200, Peter Zijlstra wrote:
> On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> > @@ -934,6 +938,21 @@ int x86_schedule_events(struct cpu_hw_events *cpuc, int n, int *assign)

> >  		for (i = 0; i < n; i++) {
> >  			e = cpuc->event_list[i];
> >  			e->hw.flags |= PERF_X86_EVENT_COMMITTED;
> > +
> > +			/*
> > +			 * 0x0300 is pseudo-encoding for REF_CPU_CYCLES.
> > +			 * It indicates that fixed counter 2 should be used.
> > +			 *
> > +			 * If fixed counter 2 is occupied and a GP counter
> > +			 * is assigned, an alternative event which can be
> > +			 * counted in GP counter will be used to replace
> > +			 * the pseudo-encoding REF_CPU_CYCLES event.
> > +			 */
> > +			if (((e->hw.config & X86_RAW_EVENT_MASK) == 0x0300) &&
> > +			    (assign[i] < INTEL_PMC_IDX_FIXED) &&
> > +			    x86_pmu.ref_cycles_rep)
> > +				x86_pmu.ref_cycles_rep(e);
> > +
> >  			if (x86_pmu.commit_scheduling)
> >  				x86_pmu.commit_scheduling(cpuc, i, assign[i]);
> >  		}
> 
> This looks dodgy, this is the branch were we managed to schedule all
> events. Why would we need to consider anything here?
> 
> I was expecting a retry if there are still unscheduled events and one of
> the events was our 0x0300 event. In that case you have to reset the
> event and retry the whole scheduling thing.

Ah, I see what you've done. That Changelog could use a lot of help, it's
barely readable.

[toc] | [prev] | [next] | [standalone]

#1647187

> On Mon, May 22, 2017 at 11:19:16AM +0200, Peter Zijlstra wrote:
> > On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> > > @@ -934,6 +938,21 @@ int x86_schedule_events(struct cpu_hw_events
> > > *cpuc, int n, int *assign)
> 
> > >  		for (i = 0; i < n; i++) {
> > >  			e = cpuc->event_list[i];
> > >  			e->hw.flags |= PERF_X86_EVENT_COMMITTED;
> > > +
> > > +			/*
> > > +			 * 0x0300 is pseudo-encoding for REF_CPU_CYCLES.
> > > +			 * It indicates that fixed counter 2 should be used.
> > > +			 *
> > > +			 * If fixed counter 2 is occupied and a GP counter
> > > +			 * is assigned, an alternative event which can be
> > > +			 * counted in GP counter will be used to replace
> > > +			 * the pseudo-encoding REF_CPU_CYCLES event.
> > > +			 */
> > > +			if (((e->hw.config & X86_RAW_EVENT_MASK) ==
> 0x0300) &&
> > > +			    (assign[i] < INTEL_PMC_IDX_FIXED) &&
> > > +			    x86_pmu.ref_cycles_rep)
> > > +				x86_pmu.ref_cycles_rep(e);
> > > +
> > >  			if (x86_pmu.commit_scheduling)
> > >  				x86_pmu.commit_scheduling(cpuc, i,
> assign[i]);
> > >  		}
> >
> > This looks dodgy, this is the branch were we managed to schedule all
> > events. Why would we need to consider anything here?
> >
> > I was expecting a retry if there are still unscheduled events and one
> > of the events was our 0x0300 event. In that case you have to reset the
> > event and retry the whole scheduling thing.
> 
> Ah, I see what you've done. That Changelog could use a lot of help, it's barely
> readable.

Thanks for the suggestions.
I will modify the changelog and make it clearer why we need the patch. 

Thanks,
Kan

[toc] | [prev] | [next] | [standalone]

#1647169

> On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index
> > 580b60f..e8b2326 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event
> *event)
> >  	delta = (new_raw_count << shift) - (prev_raw_count << shift);
> >  	delta >>= shift;
> >
> > +	/* Correct the count number if applying ref_cycles replacement */
> > +	if (!is_sampling_event(event) &&
> > +	    (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
> > +		delta *= x86_pmu.ref_cycles_factor;
> 
> That condition seems wrong, why only correct for !sampling events?
>

For sampling, it's either fixed freq mode or fixed period mode.
 - In the fixed freq mode, we should do nothing, because the adaptive
   frequency algorithm will handle it.
 - In the fixed period mode, we have already adjusted the period in 
    ref_cycles_rep().
Therefore, we should only handle !sampling events here.

 
> >  	local64_add(delta, &event->count);
> >  	local64_sub(delta, &hwc->period_left);
> >
> 
> 
> > @@ -934,6 +938,21 @@ int x86_schedule_events(struct cpu_hw_events
> *cpuc, int n, int *assign)
> >  		for (i = 0; i < n; i++) {
> >  			e = cpuc->event_list[i];
> >  			e->hw.flags |= PERF_X86_EVENT_COMMITTED;
> > +
> > +			/*
> > +			 * 0x0300 is pseudo-encoding for REF_CPU_CYCLES.
> > +			 * It indicates that fixed counter 2 should be used.
> > +			 *
> > +			 * If fixed counter 2 is occupied and a GP counter
> > +			 * is assigned, an alternative event which can be
> > +			 * counted in GP counter will be used to replace
> > +			 * the pseudo-encoding REF_CPU_CYCLES event.
> > +			 */
> > +			if (((e->hw.config & X86_RAW_EVENT_MASK) ==
> 0x0300) &&
> > +			    (assign[i] < INTEL_PMC_IDX_FIXED) &&
> > +			    x86_pmu.ref_cycles_rep)
> > +				x86_pmu.ref_cycles_rep(e);
> > +
> >  			if (x86_pmu.commit_scheduling)
> >  				x86_pmu.commit_scheduling(cpuc, i,
> assign[i]);
> >  		}
> 
> This looks dodgy, this is the branch were we managed to schedule all events.
> Why would we need to consider anything here?
> 
> I was expecting a retry if there are still unscheduled events and one of the
> events was our 0x0300 event. In that case you have to reset the event and
> retry the whole scheduling thing.

Will do it.

Thanks,
Kan

[toc] | [prev] | [next] | [standalone]

#1647286

On Mon, May 22, 2017 at 04:55:47PM +0000, Liang, Kan wrote:
> 
> 
> > On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index
> > > 580b60f..e8b2326 100644
> > > --- a/arch/x86/events/core.c
> > > +++ b/arch/x86/events/core.c
> > > @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event
> > *event)
> > >  	delta = (new_raw_count << shift) - (prev_raw_count << shift);
> > >  	delta >>= shift;
> > >
> > > +	/* Correct the count number if applying ref_cycles replacement */
> > > +	if (!is_sampling_event(event) &&
> > > +	    (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
> > > +		delta *= x86_pmu.ref_cycles_factor;
> > 
> > That condition seems wrong, why only correct for !sampling events?
> >
> 
> For sampling, it's either fixed freq mode or fixed period mode.
>  - In the fixed freq mode, we should do nothing, because the adaptive
>    frequency algorithm will handle it.
>  - In the fixed period mode, we have already adjusted the period in 
>     ref_cycles_rep().
> Therefore, we should only handle !sampling events here.

How so? For sampling events the actual event count should also be
accurate.

[toc] | [prev] | [next] | [standalone]

#1647289

On Mon, May 22, 2017 at 12:23 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, May 22, 2017 at 04:55:47PM +0000, Liang, Kan wrote:
>>
>>
>> > On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
>> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index
>> > > 580b60f..e8b2326 100644
>> > > --- a/arch/x86/events/core.c
>> > > +++ b/arch/x86/events/core.c
>> > > @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event
>> > *event)
>> > >   delta = (new_raw_count << shift) - (prev_raw_count << shift);
>> > >   delta >>= shift;
>> > >
>> > > + /* Correct the count number if applying ref_cycles replacement */
>> > > + if (!is_sampling_event(event) &&
>> > > +     (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
>> > > +         delta *= x86_pmu.ref_cycles_factor;
>> >
>> > That condition seems wrong, why only correct for !sampling events?
>> >
>>
>> For sampling, it's either fixed freq mode or fixed period mode.
>>  - In the fixed freq mode, we should do nothing, because the adaptive
>>    frequency algorithm will handle it.
>>  - In the fixed period mode, we have already adjusted the period in
>>     ref_cycles_rep().
>> Therefore, we should only handle !sampling events here.
>
> How so? For sampling events the actual event count should also be
> accurate.

Yes, it must be. Because you can reconstruct the total number of
occurrences of the event by adding
all the periods recorded in each sample. So the period in each sample
must reflect user event and not
kernel event.

[toc] | [prev] | [next] | [standalone]

#1647430

> 
> On Mon, May 22, 2017 at 12:23 PM, Peter Zijlstra <peterz@infradead.org>
> wrote:
> > On Mon, May 22, 2017 at 04:55:47PM +0000, Liang, Kan wrote:
> >>
> >>
> >> > On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> >> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> >> > > index
> >> > > 580b60f..e8b2326 100644
> >> > > --- a/arch/x86/events/core.c
> >> > > +++ b/arch/x86/events/core.c
> >> > > @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct
> perf_event
> >> > *event)
> >> > >   delta = (new_raw_count << shift) - (prev_raw_count << shift);
> >> > >   delta >>= shift;
> >> > >
> >> > > + /* Correct the count number if applying ref_cycles replacement
> >> > > + */ if (!is_sampling_event(event) &&
> >> > > +     (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
> >> > > +         delta *= x86_pmu.ref_cycles_factor;
> >> >
> >> > That condition seems wrong, why only correct for !sampling events?
> >> >
> >>
> >> For sampling, it's either fixed freq mode or fixed period mode.
> >>  - In the fixed freq mode, we should do nothing, because the adaptive
> >>    frequency algorithm will handle it.
> >>  - In the fixed period mode, we have already adjusted the period in
> >>     ref_cycles_rep().
> >> Therefore, we should only handle !sampling events here.
> >
> > How so? For sampling events the actual event count should also be
> > accurate.
> 
> Yes, it must be. Because you can reconstruct the total number of occurrences
> of the event by adding all the periods recorded in each sample. So the period
> in each sample must reflect user event and not kernel event.

Peter and Stephane, you are right.
After adjusting the period, I can only make sure the number of samples for
the bus_cycles event is the same as that for ref cycles event.
I still need to adjust the number of occurrences of the event accordingly,
to make it accurate.
I will change it in next version.

Thanks,
Kan

[toc] | [prev] | [next] | [standalone]

#1647675

On Mon, May 22, 2017 at 12:28:26PM -0700, Stephane Eranian wrote:
> On Mon, May 22, 2017 at 12:23 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, May 22, 2017 at 04:55:47PM +0000, Liang, Kan wrote:
> >>
> >>
> >> > On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
> >> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index
> >> > > 580b60f..e8b2326 100644
> >> > > --- a/arch/x86/events/core.c
> >> > > +++ b/arch/x86/events/core.c
> >> > > @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event
> >> > *event)
> >> > >   delta = (new_raw_count << shift) - (prev_raw_count << shift);
> >> > >   delta >>= shift;
> >> > >
> >> > > + /* Correct the count number if applying ref_cycles replacement */
> >> > > + if (!is_sampling_event(event) &&
> >> > > +     (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
> >> > > +         delta *= x86_pmu.ref_cycles_factor;
> >> >
> >> > That condition seems wrong, why only correct for !sampling events?
> >> >
> >>
> >> For sampling, it's either fixed freq mode or fixed period mode.
> >>  - In the fixed freq mode, we should do nothing, because the adaptive
> >>    frequency algorithm will handle it.
> >>  - In the fixed period mode, we have already adjusted the period in
> >>     ref_cycles_rep().
> >> Therefore, we should only handle !sampling events here.
> >
> > How so? For sampling events the actual event count should also be
> > accurate.
> 
> Yes, it must be. Because you can reconstruct the total number of
> occurrences of the event by adding
> all the periods recorded in each sample. So the period in each sample
> must reflect user event and not
> kernel event.

Well, that, but you can equally use read() or the mmap()'ed rdpmc stuff
on a sampling event. The fact that is also generates samples does not
mean it should not also function as a non-sampling event.

[toc] | [prev] | [next] | [standalone]

#1647682

On Mon, May 22, 2017 at 11:39 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, May 22, 2017 at 12:28:26PM -0700, Stephane Eranian wrote:
>> On Mon, May 22, 2017 at 12:23 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Mon, May 22, 2017 at 04:55:47PM +0000, Liang, Kan wrote:
>> >>
>> >>
>> >> > On Fri, May 19, 2017 at 10:06:21AM -0700, kan.liang@intel.com wrote:
>> >> > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index
>> >> > > 580b60f..e8b2326 100644
>> >> > > --- a/arch/x86/events/core.c
>> >> > > +++ b/arch/x86/events/core.c
>> >> > > @@ -101,6 +101,10 @@ u64 x86_perf_event_update(struct perf_event
>> >> > *event)
>> >> > >   delta = (new_raw_count << shift) - (prev_raw_count << shift);
>> >> > >   delta >>= shift;
>> >> > >
>> >> > > + /* Correct the count number if applying ref_cycles replacement */
>> >> > > + if (!is_sampling_event(event) &&
>> >> > > +     (hwc->flags & PERF_X86_EVENT_REF_CYCLES_REP))
>> >> > > +         delta *= x86_pmu.ref_cycles_factor;
>> >> >
>> >> > That condition seems wrong, why only correct for !sampling events?
>> >> >
>> >>
>> >> For sampling, it's either fixed freq mode or fixed period mode.
>> >>  - In the fixed freq mode, we should do nothing, because the adaptive
>> >>    frequency algorithm will handle it.
>> >>  - In the fixed period mode, we have already adjusted the period in
>> >>     ref_cycles_rep().
>> >> Therefore, we should only handle !sampling events here.
>> >
>> > How so? For sampling events the actual event count should also be
>> > accurate.
>>
>> Yes, it must be. Because you can reconstruct the total number of
>> occurrences of the event by adding
>> all the periods recorded in each sample. So the period in each sample
>> must reflect user event and not
>> kernel event.
>
> Well, that, but you can equally use read() or the mmap()'ed rdpmc stuff
> on a sampling event. The fact that is also generates samples does not
> mean it should not also function as a non-sampling event.

Right, I did not even consider the rdpmc, but yeah, you will get a count that
is not relevant to the user visible event. Unless you fake it using the time
scaling fields there but that's ugly.

[toc] | [prev] | [next] | [standalone]

#1649718

> Right, I did not even consider the rdpmc, but yeah, you will get a count that
> is not relevant to the user visible event. Unless you fake it using the time
> scaling fields there but that's ugly.

Could add another scaling field to the mmap page for this.

-Andi

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web