[MODERATED] Re: [PATCH v2 0/4] performance walnuts

On Thu, Mar 07, 2019 at 03:13:17PM +0100, speck for Greg KH wrote:
> On Wed, Mar 06, 2019 at 12:56:47PM +0100, speck for Greg KH wrote:
> > On Wed, Mar 06, 2019 at 10:23:52AM +0100, speck for Thomas Gleixner wrote:
> > > On Tue, 5 Mar 2019, speck for Peter Zijlstra wrote:
> > > 
> > > > Or whatever shiney new name they ought to get.
> > > > 
> > > > Apparently the intent is to have these patches magically appears in source
> > > > repos on the 12th.
> > > 
> > > I made them magically appear in the speck repo:
> > > 
> > >   cvs.ou.linutronix.de:linux/speck/linux tsx-5.0
> > > 
> > > and backports in the branches tsx-4.20, tsx-4.19, tsx-4.14. Git bundles
> > > are attached.
> > > 
> > > I leave the dead kernel backports to the honorable members of the Kernel
> > > Necrophilia cult as usual.
> > 
> > Many thanks for doing these backports.  I'll use them for the stable
> > updates next week.  As for kernels older than 4.14, I'll maybe try
> > 4.9 on my own...
> 
> Attached below is the 4 patches backported to 4.9.y.  They build for me,
> but I have no real way to test them (I didn't even boot them.)
> 
> If anyone cares about 4.9.y, can they test these out?  Note, all of the
> major changes was in the last patch (see my sysfs rant for why that was
> so).

I'm setting up a tree to run the same test cases we have been running on
Peter's patch set recently.

--mark

> 
> thanks,
> 
> greg k-h

> >From 123a85c589debcaa0e7f7acffba0c4c8a890f5ba Mon Sep 17 00:00:00 2001
> From: "Peter Zijlstra (Intel)" <[email protected]>
> Date: Tue, 5 Mar 2019 22:23:15 +0100
> Subject: [PATCH 1/4] perf/x86/intel: Make cpuc allocations consistent
> 
> commit d01b1f96a82e5dd7841a1d39db3abfdaf95f70ab upstream
> 
> The cpuc data structure allocation is different between fake and real
> cpuc's; use the same code to init/free both.
> 
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>  arch/x86/events/core.c       | 13 +++++--------
>  arch/x86/events/intel/core.c | 29 ++++++++++++++++++-----------
>  arch/x86/events/perf_event.h | 11 ++++++++---
>  3 files changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index ab04751a12b6..1e9f610d36a4 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1942,7 +1942,7 @@ static int x86_pmu_commit_txn(struct pmu *pmu)
>   */
>  static void free_fake_cpuc(struct cpu_hw_events *cpuc)
>  {
> -	kfree(cpuc->shared_regs);
> +	intel_cpuc_finish(cpuc);
>  	kfree(cpuc);
>  }
>  
> @@ -1954,14 +1954,11 @@ static struct cpu_hw_events *allocate_fake_cpuc(void)
>  	cpuc = kzalloc(sizeof(*cpuc), GFP_KERNEL);
>  	if (!cpuc)
>  		return ERR_PTR(-ENOMEM);
> -
> -	/* only needed, if we have extra_regs */
> -	if (x86_pmu.extra_regs) {
> -		cpuc->shared_regs = allocate_shared_regs(cpu);
> -		if (!cpuc->shared_regs)
> -			goto error;
> -	}
>  	cpuc->is_fake = 1;
> +
> +	if (intel_cpuc_prepare(cpuc, cpu))
> +		goto error;
> +
>  	return cpuc;
>  error:
>  	free_fake_cpuc(cpuc);
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index f0639c8ebcb6..6436717dbf0f 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3093,7 +3093,7 @@ ssize_t intel_event_sysfs_show(char *page, u64 config)
>  	return x86_event_sysfs_show(page, config, event);
>  }
>  
> -struct intel_shared_regs *allocate_shared_regs(int cpu)
> +static struct intel_shared_regs *allocate_shared_regs(int cpu)
>  {
>  	struct intel_shared_regs *regs;
>  	int i;
> @@ -3125,10 +3125,9 @@ static struct intel_excl_cntrs *allocate_excl_cntrs(int cpu)
>  	return c;
>  }
>  
> -static int intel_pmu_cpu_prepare(int cpu)
> -{
> -	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
>  
> +int intel_cpuc_prepare(struct cpu_hw_events *cpuc, int cpu)
> +{
>  	if (x86_pmu.extra_regs || x86_pmu.lbr_sel_map) {
>  		cpuc->shared_regs = allocate_shared_regs(cpu);
>  		if (!cpuc->shared_regs)
> @@ -3138,7 +3137,7 @@ static int intel_pmu_cpu_prepare(int cpu)
>  	if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
>  		size_t sz = X86_PMC_IDX_MAX * sizeof(struct event_constraint);
>  
> -		cpuc->constraint_list = kzalloc(sz, GFP_KERNEL);
> +		cpuc->constraint_list = kzalloc_node(sz, GFP_KERNEL, cpu_to_node(cpu));
>  		if (!cpuc->constraint_list)
>  			goto err_shared_regs;
>  
> @@ -3163,6 +3162,11 @@ static int intel_pmu_cpu_prepare(int cpu)
>  	return -ENOMEM;
>  }
>  
> +static int intel_pmu_cpu_prepare(int cpu)
> +{
> +	return intel_cpuc_prepare(&per_cpu(cpu_hw_events, cpu), cpu);
> +}
> +
>  static void intel_pmu_cpu_starting(int cpu)
>  {
>  	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
> @@ -3218,9 +3222,8 @@ static void intel_pmu_cpu_starting(int cpu)
>  	}
>  }
>  
> -static void free_excl_cntrs(int cpu)
> +static void free_excl_cntrs(struct cpu_hw_events *cpuc)
>  {
> -	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
>  	struct intel_excl_cntrs *c;
>  
>  	c = cpuc->excl_cntrs;
> @@ -3238,9 +3241,8 @@ static void intel_pmu_cpu_dying(int cpu)
>  	fini_debug_store_on_cpu(cpu);
>  }
>  
> -static void intel_pmu_cpu_dead(int cpu)
> +void intel_cpuc_finish(struct cpu_hw_events *cpuc)
>  {
> -	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
>  	struct intel_shared_regs *pc;
>  
>  	pc = cpuc->shared_regs;
> @@ -3250,7 +3252,12 @@ static void intel_pmu_cpu_dead(int cpu)
>  		cpuc->shared_regs = NULL;
>  	}
>  
> -	free_excl_cntrs(cpu);
> +	free_excl_cntrs(cpuc);
> +}
> +
> +static void intel_pmu_cpu_dead(int cpu)
> +{
> +	intel_cpuc_finish(&per_cpu(cpu_hw_events, cpu));
>  }
>  
>  static void intel_pmu_sched_task(struct perf_event_context *ctx,
> @@ -4132,7 +4139,7 @@ static __init int fixup_ht_bug(void)
>  	get_online_cpus();
>  
>  	for_each_online_cpu(c) {
> -		free_excl_cntrs(c);
> +		free_excl_cntrs(&per_cpu(cpu_hw_events, c));
>  	}
>  
>  	put_online_cpus();
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 5c21680b0a69..1ce6ae35f6a2 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -865,7 +865,8 @@ struct event_constraint *
>  x86_get_event_constraints(struct cpu_hw_events *cpuc, int idx,
>  			  struct perf_event *event);
>  
> -struct intel_shared_regs *allocate_shared_regs(int cpu);
> +extern int intel_cpuc_prepare(struct cpu_hw_events *cpuc, int cpu);
> +extern void intel_cpuc_finish(struct cpu_hw_events *cpuc);
>  
>  int intel_pmu_init(void);
>  
> @@ -995,9 +996,13 @@ static inline int intel_pmu_init(void)
>  	return 0;
>  }
>  
> -static inline struct intel_shared_regs *allocate_shared_regs(int cpu)
> +static inline int intel_cpuc_prepare(struct cpu_hw_event *cpuc, int cpu)
> +{
> +	return 0;
> +}
> +
> +static inline void intel_cpuc_finish(struct cpu_hw_event *cpuc)
>  {
> -	return NULL;
>  }
>  
>  static inline int is_ht_workaround_enabled(void)
> -- 
> 2.21.0
> 

> >From d09645273ba760b3f1f50216482a7eb137e3ea1c Mon Sep 17 00:00:00 2001
> From: "Peter Zijlstra (Intel)" <[email protected]>
> Date: Tue, 5 Mar 2019 22:23:16 +0100
> Subject: [PATCH 2/4] perf/x86/intel: Generalize dynamic constraint creation
> 
> commit 11f8b2d65ca9029591c8df26bb6bd063c312b7fe upstream
> 
> Such that we can re-use it.
> 
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>  arch/x86/events/intel/core.c | 51 +++++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 6436717dbf0f..098ab775135f 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2492,6 +2492,35 @@ intel_stop_scheduling(struct cpu_hw_events *cpuc)
>  	raw_spin_unlock(&excl_cntrs->lock);
>  }
>  
> +static struct event_constraint *
> +dyn_constraint(struct cpu_hw_events *cpuc, struct event_constraint *c, int idx)
> +{
> +	WARN_ON_ONCE(!cpuc->constraint_list);
> +
> +	if (!(c->flags & PERF_X86_EVENT_DYNAMIC)) {
> +		struct event_constraint *cx;
> +
> +		/*
> +		 * grab pre-allocated constraint entry
> +		 */
> +		cx = &cpuc->constraint_list[idx];
> +
> +		/*
> +		 * initialize dynamic constraint
> +		 * with static constraint
> +		 */
> +		*cx = *c;
> +
> +		/*
> +		 * mark constraint as dynamic
> +		 */
> +		cx->flags |= PERF_X86_EVENT_DYNAMIC;
> +		c = cx;
> +	}
> +
> +	return c;
> +}
> +
>  static struct event_constraint *
>  intel_get_excl_constraints(struct cpu_hw_events *cpuc, struct perf_event *event,
>  			   int idx, struct event_constraint *c)
> @@ -2522,27 +2551,7 @@ intel_get_excl_constraints(struct cpu_hw_events *cpuc, struct perf_event *event,
>  	 * only needed when constraint has not yet
>  	 * been cloned (marked dynamic)
>  	 */
> -	if (!(c->flags & PERF_X86_EVENT_DYNAMIC)) {
> -		struct event_constraint *cx;
> -
> -		/*
> -		 * grab pre-allocated constraint entry
> -		 */
> -		cx = &cpuc->constraint_list[idx];
> -
> -		/*
> -		 * initialize dynamic constraint
> -		 * with static constraint
> -		 */
> -		*cx = *c;
> -
> -		/*
> -		 * mark constraint as dynamic, so we
> -		 * can free it later on
> -		 */
> -		cx->flags |= PERF_X86_EVENT_DYNAMIC;
> -		c = cx;
> -	}
> +	c = dyn_constraint(cpuc, c, idx);
>  
>  	/*
>  	 * From here on, the constraint is dynamic.
> -- 
> 2.21.0
> 

> >From 1359faf0754b28ff8e94fee78f2c2b7d69c4b0d3 Mon Sep 17 00:00:00 2001
> From: "Peter Zijlstra (Intel)" <[email protected]>
> Date: Tue, 5 Mar 2019 22:23:17 +0100
> Subject: [PATCH 3/4] x86: Add TSX Force Abort CPUID/MSR
> 
> commit 52f64909409c17adf54fcf5f9751e0544ca3a6b4 upstream
> 
> Skylake systems will receive a microcode update to address a TSX
> errata. This microcode will (by default) clobber PMC3 when TSX
> instructions are (speculatively or not) executed.
> 
> It also provides an MSR to cause all TSX transaction to abort and
> preserve PMC3.
> 
> Add the CPUID enumeration and MSR definition.
> 
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>  arch/x86/include/asm/cpufeatures.h | 1 +
>  arch/x86/include/asm/msr-index.h   | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index c56c24347f15..98444b77fbe3 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -314,6 +314,7 @@
>  /* Intel-defined CPU features, CPUID level 0x00000007:0 (EDX), word 18 */
>  #define X86_FEATURE_AVX512_4VNNIW	(18*32+ 2) /* AVX-512 Neural Network Instructions */
>  #define X86_FEATURE_AVX512_4FMAPS	(18*32+ 3) /* AVX-512 Multiply Accumulation Single precision */
> +#define X86_FEATURE_TSX_FORCE_ABORT	(18*32+13) /* "" TSX_FORCE_ABORT */
>  #define X86_FEATURE_PCONFIG		(18*32+18) /* Intel PCONFIG */
>  #define X86_FEATURE_SPEC_CTRL		(18*32+26) /* "" Speculation Control (IBRS + IBPB) */
>  #define X86_FEATURE_INTEL_STIBP		(18*32+27) /* "" Single Thread Indirect Branch Predictors */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index bbbb9b14ade1..9963e21ac443 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -575,6 +575,12 @@
>  
>  #define MSR_IA32_TSC_DEADLINE		0x000006E0
>  
> +
> +#define MSR_TSX_FORCE_ABORT		0x0000010F
> +
> +#define MSR_TFA_RTM_FORCE_ABORT_BIT	0
> +#define MSR_TFA_RTM_FORCE_ABORT		BIT_ULL(MSR_TFA_RTM_FORCE_ABORT_BIT)
> +
>  /* P4/Xeon+ specific */
>  #define MSR_IA32_MCG_EAX		0x00000180
>  #define MSR_IA32_MCG_EBX		0x00000181
> -- 
> 2.21.0
> 

> >From 78289b2693ddb615d32514fa5915f46d941e526d Mon Sep 17 00:00:00 2001
> From: "Peter Zijlstra (Intel)" <[email protected]>
> Date: Tue, 5 Mar 2019 22:23:18 +0100
> Subject: [PATCH 4/4] perf/x86/intel: Implement support for TSX Force Abort
> 
> commit 400816f60c543153656ac74eaf7f36f6b7202378 upstream
> 
> Skylake (and later) will receive a microcode update to address a TSX
> errata. This microcode will, on execution of a TSX instruction
> (speculative or not) use (clobber) PMC3. This update will also provide
> a new MSR to change this behaviour along with a CPUID bit to enumerate
> the presence of this new MSR.
> 
> When the MSR gets set; the microcode will no longer use PMC3 but will
> Force Abort every TSX transaction (upon executing COMMIT).
> 
> When TSX Force Abort (TFA) is allowed (default); the MSR gets set when
> PMC3 gets scheduled and cleared when, after scheduling, PMC3 is
> unused.
> 
> When TFA is not allowed; clear PMC3 from all constraints such that it
> will not get used.
> 
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>  arch/x86/events/intel/core.c | 75 ++++++++++++++++++++++++++++++++++--
>  arch/x86/events/perf_event.h |  6 +++
>  2 files changed, 78 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 098ab775135f..2f5440084951 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -1860,6 +1860,39 @@ static void intel_pmu_nhm_enable_all(int added)
>  	intel_pmu_enable_all(added);
>  }
>  
> +static void intel_set_tfa(struct cpu_hw_events *cpuc, bool on)
> +{
> +	u64 val = on ? MSR_TFA_RTM_FORCE_ABORT : 0;
> +
> +	if (cpuc->tfa_shadow != val) {
> +		cpuc->tfa_shadow = val;
> +		wrmsrl(MSR_TSX_FORCE_ABORT, val);
> +	}
> +}
> +
> +static void intel_tfa_commit_scheduling(struct cpu_hw_events *cpuc, int idx, int cntr)
> +{
> +	/*
> +	 * We're going to use PMC3, make sure TFA is set before we touch it.
> +	 */
> +	if (cntr == 3 && !cpuc->is_fake)
> +		intel_set_tfa(cpuc, true);
> +}
> +
> +static void intel_tfa_pmu_enable_all(int added)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +
> +	/*
> +	 * If we find PMC3 is no longer used when we enable the PMU, we can
> +	 * clear TFA.
> +	 */
> +	if (!test_bit(3, cpuc->active_mask))
> +		intel_set_tfa(cpuc, false);
> +
> +	intel_pmu_enable_all(added);
> +}
> +
>  static inline u64 intel_pmu_get_status(void)
>  {
>  	u64 status;
> @@ -3049,6 +3082,27 @@ hsw_get_event_constraints(struct cpu_hw_events *cpuc, int idx,
>  	return c;
>  }
>  
> +static bool allow_tsx_force_abort = true;
> +static DEVICE_BOOL_ATTR(allow_tsx_force_abort, 0644, allow_tsx_force_abort);
> +
> +static struct event_constraint *
> +tfa_get_event_constraints(struct cpu_hw_events *cpuc, int idx,
> +			  struct perf_event *event)
> +{
> +	struct event_constraint *c = hsw_get_event_constraints(cpuc, idx, event);
> +
> +	/*
> +	 * Without TFA we must not use PMC3.
> +	 */
> +	if (!allow_tsx_force_abort && test_bit(3, c->idxmsk)) {
> +		c = dyn_constraint(cpuc, c, idx);
> +		c->idxmsk64 &= ~(1ULL << 3);
> +		c->weight--;
> +	}
> +
> +	return c;
> +}
> +
>  /*
>   * Broadwell:
>   *
> @@ -3143,13 +3197,15 @@ int intel_cpuc_prepare(struct cpu_hw_events *cpuc, int cpu)
>  			goto err;
>  	}
>  
> -	if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
> +	if (x86_pmu.flags & (PMU_FL_EXCL_CNTRS | PMU_FL_TFA)) {
>  		size_t sz = X86_PMC_IDX_MAX * sizeof(struct event_constraint);
>  
>  		cpuc->constraint_list = kzalloc_node(sz, GFP_KERNEL, cpu_to_node(cpu));
>  		if (!cpuc->constraint_list)
>  			goto err_shared_regs;
> +	}
>  
> +	if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
>  		cpuc->excl_cntrs = allocate_excl_cntrs(cpu);
>  		if (!cpuc->excl_cntrs)
>  			goto err_constraint_list;
> @@ -3240,9 +3296,10 @@ static void free_excl_cntrs(struct cpu_hw_events *cpuc)
>  		if (c->core_id == -1 || --c->refcnt == 0)
>  			kfree(c);
>  		cpuc->excl_cntrs = NULL;
> -		kfree(cpuc->constraint_list);
> -		cpuc->constraint_list = NULL;
>  	}
> +
> +	kfree(cpuc->constraint_list);
> +	cpuc->constraint_list = NULL;
>  }
>  
>  static void intel_pmu_cpu_dying(int cpu)
> @@ -3626,6 +3683,7 @@ static struct attribute *hsw_events_attrs[] = {
>  	EVENT_PTR(td_total_slots_scale),
>  	EVENT_PTR(td_recovery_bubbles),
>  	EVENT_PTR(td_recovery_bubbles_scale),
> +	NULL, /* &dev_attr_allow_tsx_force_abort.attr.attr */
>  	NULL
>  };
>  
> @@ -4021,6 +4079,17 @@ __init int intel_pmu_init(void)
>  						  skl_format_attr);
>  		WARN_ON(!x86_pmu.format_attrs);
>  		x86_pmu.cpu_events = hsw_events_attrs;
> +
> +		if (boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) {
> +			x86_pmu.flags |= PMU_FL_TFA;
> +			x86_pmu.get_event_constraints = tfa_get_event_constraints;
> +			x86_pmu.enable_all = intel_tfa_pmu_enable_all;
> +			x86_pmu.commit_scheduling = intel_tfa_commit_scheduling;
> +			for (i = 0; hsw_events_attrs[i] != NULL; i++)
> +				;
> +			hsw_events_attrs[i] = &dev_attr_allow_tsx_force_abort.attr.attr;
> +		}
> +
>  		pr_cont("Skylake events, ");
>  		break;
>  
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 1ce6ae35f6a2..d3bbabb7b520 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -230,6 +230,11 @@ struct cpu_hw_events {
>  	struct intel_excl_cntrs		*excl_cntrs;
>  	int excl_thread_id; /* 0 or 1 */
>  
> +	/*
> +	 * SKL TSX_FORCE_ABORT shadow
> +	 */
> +	u64				tfa_shadow;
> +
>  	/*
>  	 * AMD specific bits
>  	 */
> @@ -659,6 +664,7 @@ do {									\
>  #define PMU_FL_HAS_RSP_1	0x2 /* has 2 equivalent offcore_rsp regs   */
>  #define PMU_FL_EXCL_CNTRS	0x4 /* has exclusive counter requirements  */
>  #define PMU_FL_EXCL_ENABLED	0x8 /* exclusive counter active */
> +#define PMU_FL_TFA		0x20 /* deal with TSX force abort */
>  
>  #define EVENT_VAR(_id)  event_attr_##_id
>  #define EVENT_PTR(_id) &event_attr_##_id.attr.attr
> -- 
> 2.21.0
>