From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (146.0.238.70:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 08 Feb 2019 18:07:58 -0000 Received: from mga05.intel.com ([192.55.52.43]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1gsAZA-0000PP-KM for speck@linutronix.de; Fri, 08 Feb 2019 19:07:57 +0100 Date: Fri, 8 Feb 2019 10:07:53 -0800 From: Andi Kleen Subject: [MODERATED] Re: [RFC][PATCH] performance walnuts Message-ID: <20190208180753.GC16922@tassilo.jf.intel.com> References: <3dd5d6e2bc9ac53f826c251c68ce84fcc79a6872.1549582769.git.ak@linux.intel.com> <20190208090147.GK32477@hirez.programming.kicks-ass.net> <20190208093950.GD32534@hirez.programming.kicks-ass.net> <20190208105318.GE32534@hirez.programming.kicks-ass.net> MIME-Version: 1.0 In-Reply-To: <20190208105318.GE32534@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: 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