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

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Feb 08, 2019 at 10:07:53AM -0800, speck for Andi Kleen wrote:
> On Fri, Feb 08, 2019 at 11:53:18AM +0100, speck for Peter Zijlstra wrote:

> >  	 * 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.

This is what Thomas wants and makes sense to me. I'm done talking about
this.

> > +
> > +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.

It needs no such thing. Userspace has the one knob.

And I want to hear from Thomas and Paolo on what KVM wants; Andrew
already said he doesn't want this crap exposed to virt. And the less I
have to worry about virt the happier I am.

I've yet to see compelling arguments for making this more complicated.

Minimal and correct are the name of the game here; we need to backport
this because that fscking ucode default screws us over.

If you want complicated; you can try arguing that after we've gone
public.

> > @@ -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.

This is more explicit; the 'feature' is tsx_force_abort and will be
called so in /proc/cpuid.

> >  static struct attribute *intel_pmu_attrs[] = {
> >  	&dev_attr_freeze_on_smi.attr,
> >  	NULL,
> 
> This needs a comment, as done in my patch.

Sure can do.

> > +	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.

Already gone.

> >  		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.

Then WTH expose us to it in the first place?