[MODERATED] Re: [RFC][PATCH] performance walnuts

[Andi Kleen <ak@linux.intel.com>]

On Fri, Feb 08, 2019 at 11:53:18AM +0100, speck for Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:39:50AM +0100, Peter Zijlstra wrote:
> > Ah, I think I found a way to avoid having to rely on this. Let me try.
> 
> Something like so. Can someone with access to a relevant machine test
> this?
> 
> If it works, I'll write a Changelog and this'll be it.

I'll test this, but I would appreciate it if you spin
new versions you at least merge the fixes from my patchkits
first.

This is missing several.

Also note that some of the other patches have also changed.

>  	 * From here on, the constraint is dynamic.
> @@ -3345,6 +3387,26 @@ glp_get_event_constraints(struct cpu_hw_events *cpuc, int idx,
>  	return c;
>  }
>  
> +static bool allow_tsx_force_abort = true;

The default needs more discussion.


> +
> +static struct event_constraint *
> +skl_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)) {

This still needs the extra changes in my patchkit to allow user/kvm opt-in/out.


> @@ -4061,9 +4126,12 @@ static struct attribute *intel_pmu_caps_attrs[] = {
>         NULL
>  };
>  
> +DEVICE_BOOL_ATTR(allow_tsx_force_abort, 0644, allow_tsx_force_abort);


I still think "enable_all_counters" is a far better name.

> +
>  static struct attribute *intel_pmu_attrs[] = {
>  	&dev_attr_freeze_on_smi.attr,
>  	NULL,

This needs a comment, as done in my patch.

> +	NULL,
>  };
>  
>  static __init struct attribute **
> @@ -4546,6 +4614,7 @@ __init int intel_pmu_init(void)
>  		x86_pmu.flags |= PMU_FL_HAS_RSP_1;
>  		x86_pmu.flags |= PMU_FL_NO_HT_SHARING;
>  
> +

Unnecessary change.

>  		x86_pmu.hw_config = hsw_hw_config;
>  		x86_pmu.get_event_constraints = hsw_get_event_constraints;
>  		extra_attr = boot_cpu_has(X86_FEATURE_RTM) ?
> @@ -4557,6 +4626,16 @@ __init int intel_pmu_init(void)
>  		tsx_attr = hsw_tsx_events_attrs;
>  		intel_pmu_pebs_data_source_skl(
>  			boot_cpu_data.x86_model == INTEL_FAM6_SKYLAKE_X);
> +
> +		/* If our CPU haz a walnut */
> +		if (boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) {
> +			x86_pmu.flags |= PMU_FL_WALNUT;

I don't think the Walnut name will be publicly documented, so it will be just confusing.
Better to give it an descriptive name. TSX_COUNTER3 or something like this.


-Andi